[{"id":21929,"web_url":"https://patchwork.libcamera.org/comment/21929/","msgid":"<164124902101.682966.7335271804739532693@Monstersaurus>","date":"2022-01-03T22:30:21","subject":"Re: [libcamera-devel] [PATCH v2 3/3] android: Plumb noise reduction","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2021-12-21 05:10:23)\n> Add:\n> - hardware level detection based on available noise reduction values\n> (no specific capabilities seem to require it; only FULL in general)\n> - Conversion from android to libcamera noise reduction modes for request\n> (use switch-case instead of direct copy just in case)\n> - Conversion from libcamera to android noise reduction modes for result\n> - noise reduction values to the templates\n> \n> We already have the mechanism to report the available noise reduction\n> modes, so that is not added.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v2:\n> - change result metadata generation to switch-case\n> ---\n> src/android/camera_capabilities.cpp | 34 +++++++++++++-\n> src/android/camera_device.cpp | 70 +++++++++++++++++++++++++++--\n> 2 files changed, 100 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index d6a1589e..365937ed 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -412,6 +412,31 @@ void CameraCapabilities::computeHwLevel(\n> hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> }\n> \n> + found = staticMetadata_->entryContains(\n> + ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> + ANDROID_NOISE_REDUCTION_MODE_OFF);\n> + if (!found) {\n> + LOG(HAL, Info) << noFull << \"missing noise reduction mode off\";\n> + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> + }\n> +\n> + found = staticMetadata_->entryContains(\n> + ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> + ANDROID_NOISE_REDUCTION_MODE_FAST);\n> + if (!found) {\n> + LOG(HAL, Info) << noFull << \"missing noise reduction mode fast\";\n> + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> + }\n> +\n> + /* Android docs don't say this is required, but CTS does. */\n> + found = staticMetadata_->entryContains(\n> + ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> + ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY);\n> + if (!found) {\n> + LOG(HAL, Info) << noFull << \"missing noise reduction mode high quality\";\n> + hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;\n> + }\n> +\n> hwLevel_ = hwLevel;\n> }\n> \n> @@ -1727,7 +1752,7 @@ std::unique_ptr CameraCapabilities::requestTemplatePreview() con\n> requestTemplate->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,\n> faceDetectMode);\n> \n> - uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_OFF;\n> + uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_FAST;\n> requestTemplate->addEntry(ANDROID_NOISE_REDUCTION_MODE,\n> noiseReduction);\n> \n> @@ -1764,6 +1789,13 @@ std::unique_ptr CameraCapabilities::requestTemplateStill() const\n> stillTemplate->appendEntry(ANDROID_EDGE_MODE, edgeMode);\n> }\n> \n> + if (staticMetadata_->entryContains(\n> + ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,\n> + ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY)) {\n> + uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;\n> + stillTemplate->appendEntry(ANDROID_NOISE_REDUCTION_MODE, noiseReduction);\n> + }\n> +\n> return stillTemplate;\n> }\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 8861447d..e521ae0a 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -920,6 +920,39 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> controls.set(controls::DigitalGain, lastDigitalGain_);\n> }\n> \n> + if (settings.getEntry(ANDROID_NOISE_REDUCTION_MODE, &entry)) {\n> + const uint8_t data = *entry.data.u8;\n> + int32_t noiseReductionMode;\n> + switch (data) {\n> + case ANDROID_NOISE_REDUCTION_MODE_OFF:\n> + noiseReductionMode = controls::NoiseReductionModeOff;\n> + break;\n> +\n> + case ANDROID_NOISE_REDUCTION_MODE_FAST:\n> + noiseReductionMode = controls::NoiseReductionModeFast;\n> + break;\n> +\n> + case ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY:\n> + noiseReductionMode = controls::NoiseReductionModeHighQuality;\n> + break;\n> +\n> + case ANDROID_NOISE_REDUCTION_MODE_MINIMAL:\n> + noiseReductionMode = controls::NoiseReductionModeRaw;\n> + break;\n> +\n> + case ANDROID_NOISE_REDUCTION_MODE_ZERO_SHUTTER_LAG:\n> + noiseReductionMode = controls::NoiseReductionModeZSL;\n> + break;\n> +\n> + default:\n> + LOG(HAL, Error)\n> + << \"Unknown noise reduction mode: \" << data;\n> + return -EINVAL;\n\nIs it defined/documented that one control failing should stop processing\nall other controls? or should it try to handle the other controls as\nbest as it could too?\n\nI guess a failure is a failure all the same.\n\n> + }\n> +\n> + controls.set(controls::NoiseReductionMode, noiseReductionMode);\n> + }\n> +\n\nI wonder if those one-to-one mappings would be cleaner in a map ... but\nI don't think that's essential.\n\n> if (settings.getEntry(ANDROID_EDGE_MODE, &entry)) {\n> const auto &info = camera_->controls().find(&controls::Sharpness);\n> if (info != camera_->controls().end()) {\n> @@ -1587,9 +1620,6 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> value = ANDROID_STATISTICS_SCENE_FLICKER_NONE;\n> resultMetadata->addEntry(ANDROID_STATISTICS_SCENE_FLICKER, value);\n> \n> - value = ANDROID_NOISE_REDUCTION_MODE_OFF;\n> - resultMetadata->addEntry(ANDROID_NOISE_REDUCTION_MODE, value);\n> -\n> /* 33.3 msec */\n> const int64_t rolling_shutter_skew = 33300000;\n> resultMetadata->addEntry(ANDROID_SENSOR_ROLLING_SHUTTER_SKEW,\n> @@ -1643,6 +1673,40 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> resultMetadata->addEntry(ANDROID_SCALER_CROP_REGION, cropRect);\n> }\n> \n> + if (metadata.contains(controls::NoiseReductionMode)) {\n> + bool valid;\n> + switch (metadata.get(controls::NoiseReductionMode)) {\n> + case controls::NoiseReductionModeOff:\n> + value = ANDROID_NOISE_REDUCTION_MODE_OFF;\n> + valid = true;\n> + break;\n> + case controls::NoiseReductionModeFast:\n> + value = ANDROID_NOISE_REDUCTION_MODE_FAST;\n> + valid = true;\n> + break;\n> + case controls::NoiseReductionModeHighQuality:\n> + value = ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY;\n> + valid = true;\n> + break;\n> + case controls::NoiseReductionModeRaw:\n> + value = ANDROID_NOISE_REDUCTION_MODE_MINIMAL;\n> + valid = true;\n> + break;\n> + case controls::NoiseReductionModeZSL:\n> + value = ANDROID_NOISE_REDUCTION_MODE_ZERO_SHUTTER_LAG;\n> + valid = true;\n> + break;\n> + default:\n> + LOG(HAL, Error)\n> + << \"Unknown noise reduction mode\";\n> + valid = false;\n> + }\n> +\n> + /* Can be null on non-FULL */\n\nDoes that mean non-FULL will print 'unknown noise reduction mode' on\nevery frame?\n\n\n> + if (valid)\n> + resultMetadata->addEntry(ANDROID_NOISE_REDUCTION_MODE, value);\n\nHrm though, here a map might be far more concise here, as then it would\nonly addEntry if the key was found.\n\n\tif (metadata.contains(controls::NoiseReductionMode)) {\n\t\tstatic std::map modes = {\n\t\t\t{ controls::NoiseReductionModeOff, ANDROID_NOISE_REDUCTION_MODE_OFF },\n\t\t\t{ controls::NoiseReductionModeFast, ANDROID_NOISE_REDUCTION_MODE_FAST },\n\t\t\t{ controls::NoiseReductionModeHighQuality, ANDROID_NOISE_REDUCTION_MODE_HIGH_QUALITY },\n\t\t\t{ controls::NoiseReductionModeMinimal, ANDROID_NOISE_REDUCTION_MODE_MINIMAL },\n\t\t\t{ controls::NoiseReductionModeZSL, ANDROID_NOISE_REDUCTION_MODE_ZERO_SHUTTER_LAG },\n\t\t};\n\n\t\tauto it = modes.find(metadata.get(controls::NoiseReductionMode);\n\t\tif (it != modes.end())\n\t\t\tresultMetadata->addEntry(ANDROID_NOISE_REDUCTION_MODE, it->second);\n\t\telse\n\t\t\tLOG(HAL, Error) << \"Unknown noise reduction mode\";\n\t}\n\n(I know that goes up to 112 chars there, but I think it's better\nformatted with lines as a table)\n\nAnyway, I see you changed this specifically to a switch case here\nbefore, so either way:\n\nReviewed-by: Kieran Bingham \n\n> + }\n> +\n> if (metadata.contains(controls::draft::TestPatternMode)) {\n> const int32_t testPatternMode =\n> metadata.get(controls::draft::TestPatternMode);\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 B42BDBF415\n\tfor ;\n\tMon, 3 Jan 2022 22:30:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D51AA6091D;\n\tMon, 3 Jan 2022 23:30:25 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB7B16021A\n\tfor ;\n\tMon, 3 Jan 2022 23:30:23 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 50A28E53;\n\tMon, 3 Jan 2022 23:30:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OrAxsFlK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1641249023;\n\tbh=oFxrunyYVtLmVMKgcIUuhchI0raY1v1C1Athb/HNxgc=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=OrAxsFlKfo2M/XEVAM/GULnrX/0p8UcVzczSNsrzfk9ZOssNrkhi7PqqVtVpLLJro\n\tpyCi0Ui9X75d8bRzNU5ytCZ6B3gcmfo+79O04Vu2TD8RPwSuiykr2PCj6ma5qPm1Z1\n\t5yMxBwuAnc1YTBgTIqzNhdY1jhz4GODKHdf50POg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211221051023.2628625-4-paul.elder@ideasonboard.com>","References":"<20211221051023.2628625-1-paul.elder@ideasonboard.com>\n\t<20211221051023.2628625-4-paul.elder@ideasonboard.com>","From":"Kieran Bingham ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 03 Jan 2022 22:30:21 +0000","Message-ID":"<164124902101.682966.7335271804739532693@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 3/3] android: Plumb noise reduction","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\" "}}]