[{"id":14823,"web_url":"https://patchwork.libcamera.org/comment/14823/","msgid":"","date":"2021-01-27T10:09:27","subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 26/01/2021 15:47, David Plowman wrote:\n> Users are free to avoid loading certain control algorithms from the\n> json tuning file if they wish. Under such circumstances, failing\n> completely when an application tries to set parameters for that\n> algorithm is unhelpful, and it is better just to issue a warning.\n\nAha this looks familiar from some review comments recently.\n\nCertainly if the user can disable these, making sure we don't assert is\na good thing.\n\nI wondered reading this pathc if it would help if the ControlList would\nbe passed into the algorithms with a dedicated operation so each\nalgorithm could parse the controls relevant to it, but then we'd lose\nthe ability to report a warning if controls were not processed.\n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----\n> 1 file changed, 72 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 5ccc7a65..b57d77e9 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tswitch (ctrl.first) {\n> \t\tcase controls::AE_ENABLE: {\n> \t\t\tRPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AE_ENABLE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> \t\t\tif (ctrl.second.get() == false)\n> \t\t\t\tagc->Pause();\n> \t\t\telse\n> @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::EXPOSURE_TIME: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set EXPOSURE_TIME - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\t/* This expects units of micro-seconds. */\n> \t\t\tagc->SetFixedShutter(ctrl.second.get());\n> @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::ANALOGUE_GAIN: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set ANALOGUE_GAIN - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> \t\t\tagc->SetFixedAnalogueGain(ctrl.second.get());\n> \n> \t\t\tlibcameraMetadata_.set(controls::AnalogueGain,\n> @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::AE_METERING_MODE: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AE_METERING_MODE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tint32_t idx = ctrl.second.get();\n> \t\t\tif (MeteringModeTable.count(idx)) {\n> @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::AE_CONSTRAINT_MODE: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AE_CONSTRAINT_MODE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tint32_t idx = ctrl.second.get();\n> \t\t\tif (ConstraintModeTable.count(idx)) {\n> @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::AE_EXPOSURE_MODE: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AE_EXPOSURE_MODE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tint32_t idx = ctrl.second.get();\n> \t\t\tif (ExposureModeTable.count(idx)) {\n> @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::EXPOSURE_VALUE: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set EXPOSURE_VALUE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\t/*\n> \t\t\t * The SetEv() method takes in a direct exposure multiplier.\n> @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \n> \t\tcase controls::AWB_ENABLE: {\n> \t\t\tRPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> -\t\t\tASSERT(awb);\n> +\t\t\tif (!awb) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AWB_ENABLE - no AWB algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tif (ctrl.second.get() == false)\n> \t\t\t\tawb->Pause();\n> @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::AWB_MODE: {\n> \t\t\tRPiController::AwbAlgorithm *awb = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"awb\"));\n> -\t\t\tASSERT(awb);\n> +\t\t\tif (!awb) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AWB_MODE - no AWB algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tint32_t idx = ctrl.second.get();\n> \t\t\tif (AwbModeTable.count(idx)) {\n> @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\tauto gains = ctrl.second.get>();\n> \t\t\tRPiController::AwbAlgorithm *awb = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"awb\"));\n> -\t\t\tASSERT(awb);\n> +\t\t\tif (!awb) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set COLOUR_GAINS - no AWB algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tawb->SetManualGains(gains[0], gains[1]);\n> \t\t\tif (gains[0] != 0.0f && gains[1] != 0.0f)\n> @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::BRIGHTNESS: {\n> \t\t\tRPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"contrast\"));\n> -\t\t\tASSERT(contrast);\n> +\t\t\tif (!contrast) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set BRIGHTNESS - no contrast algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tcontrast->SetBrightness(ctrl.second.get() * 65536);\n> \t\t\tlibcameraMetadata_.set(controls::Brightness,\n> @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::CONTRAST: {\n> \t\t\tRPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"contrast\"));\n> -\t\t\tASSERT(contrast);\n> +\t\t\tif (!contrast) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set CONTRAST - no contrast algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tcontrast->SetContrast(ctrl.second.get());\n> \t\t\tlibcameraMetadata_.set(controls::Contrast,\n> @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::SATURATION: {\n> \t\t\tRPiController::CcmAlgorithm *ccm = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"ccm\"));\n> -\t\t\tASSERT(ccm);\n> +\t\t\tif (!ccm) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set SATURATION - no ccm algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tccm->SetSaturation(ctrl.second.get());\n> \t\t\tlibcameraMetadata_.set(controls::Saturation,\n> @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::SHARPNESS: {\n> \t\t\tRPiController::SharpenAlgorithm *sharpen = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"sharpen\"));\n> -\t\t\tASSERT(sharpen);\n> +\t\t\tif (!sharpen) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set SHARPNESS - no sharpen algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tsharpen->SetStrength(ctrl.second.get());\n> \t\t\tlibcameraMetadata_.set(controls::Sharpness,\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 38629BD808\n\tfor ;\n\tWed, 27 Jan 2021 10:09:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 08DFD68362;\n\tWed, 27 Jan 2021 11:09:32 +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 A62426835B\n\tfor ;\n\tWed, 27 Jan 2021 11:09:30 +0100 (CET)","from [192.168.0.20]\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 421CF240;\n\tWed, 27 Jan 2021 11:09:30 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ExMfMQMf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611742170;\n\tbh=GdHV3Duz278H0cdllH7o4V6ZdbrnZpWxwAdAA5ssRXs=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=ExMfMQMfYZSaZCmGy8KpXlBQr5Y1PPwj8wVAht0f4cMl35txWpMqA8CDRzuQQnQbw\n\tzL7fe0xdHjfBNK6qpYcqlYdm6cORW/H97vZjYFKJv3Jdl4KuKm0xu5dY4jXbnQ83xj\n\tRIiYUMUexnEqS5MoEEJEcfcQ3WVQXqdPgEfOgvVs=","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210126154724.7155-1-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Wed, 27 Jan 2021 10:09:27 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20210126154724.7155-1-david.plowman@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14824,"web_url":"https://patchwork.libcamera.org/comment/14824/","msgid":"","date":"2021-01-27T10:18:44","subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:\n> Users are free to avoid loading certain control algorithms from the\n> json tuning file if they wish. Under such circumstances, failing\n> completely when an application tries to set parameters for that\n> algorithm is unhelpful, and it is better just to issue a warning.\n> \n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----\n> 1 file changed, 72 insertions(+), 14 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 5ccc7a65..b57d77e9 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tswitch (ctrl.first) {\n> \t\tcase controls::AE_ENABLE: {\n> \t\t\tRPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AE_ENABLE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n\nThis is a better behaviour indeed. I wonder if we need some kind of\nLOG_ONCE() to avoid flooding the log, but it would be difficult to do so\nper-camera.\n\nNot aborting when a control is set without a corresponding algorithm is\ngood, but I think we need to go one step further and not report the\ncontrol to the application in the first place. This will require\nreplacing the static ControlInfoMap in\ninclude/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We\nshould then extend the libcamera core to verify that all controls in a\nrequest are supported and either reject the request or strip the\nunsupported controls, and we won't need this patch anymore.\n\nI'm fine merging this patch as-is, given that the ongoing IPA IPC work\nshould make it easier to handle controls dynamically when it will land,\nbut it should then probably be reverted.\n\n> +\n> \t\t\tif (ctrl.second.get() == false)\n> \t\t\t\tagc->Pause();\n> \t\t\telse\n> @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::EXPOSURE_TIME: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set EXPOSURE_TIME - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\t/* This expects units of micro-seconds. */\n> \t\t\tagc->SetFixedShutter(ctrl.second.get());\n> @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::ANALOGUE_GAIN: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set ANALOGUE_GAIN - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> \t\t\tagc->SetFixedAnalogueGain(ctrl.second.get());\n> \n> \t\t\tlibcameraMetadata_.set(controls::AnalogueGain,\n> @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::AE_METERING_MODE: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AE_METERING_MODE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tint32_t idx = ctrl.second.get();\n> \t\t\tif (MeteringModeTable.count(idx)) {\n> @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::AE_CONSTRAINT_MODE: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AE_CONSTRAINT_MODE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tint32_t idx = ctrl.second.get();\n> \t\t\tif (ConstraintModeTable.count(idx)) {\n> @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::AE_EXPOSURE_MODE: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AE_EXPOSURE_MODE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tint32_t idx = ctrl.second.get();\n> \t\t\tif (ExposureModeTable.count(idx)) {\n> @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::EXPOSURE_VALUE: {\n> \t\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> -\t\t\tASSERT(agc);\n> +\t\t\tif (!agc) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set EXPOSURE_VALUE - no AGC algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\t/*\n> \t\t\t * The SetEv() method takes in a direct exposure multiplier.\n> @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \n> \t\tcase controls::AWB_ENABLE: {\n> \t\t\tRPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> -\t\t\tASSERT(awb);\n> +\t\t\tif (!awb) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AWB_ENABLE - no AWB algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tif (ctrl.second.get() == false)\n> \t\t\t\tawb->Pause();\n> @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::AWB_MODE: {\n> \t\t\tRPiController::AwbAlgorithm *awb = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"awb\"));\n> -\t\t\tASSERT(awb);\n> +\t\t\tif (!awb) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set AWB_MODE - no AWB algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tint32_t idx = ctrl.second.get();\n> \t\t\tif (AwbModeTable.count(idx)) {\n> @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\tauto gains = ctrl.second.get>();\n> \t\t\tRPiController::AwbAlgorithm *awb = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"awb\"));\n> -\t\t\tASSERT(awb);\n> +\t\t\tif (!awb) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set COLOUR_GAINS - no AWB algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tawb->SetManualGains(gains[0], gains[1]);\n> \t\t\tif (gains[0] != 0.0f && gains[1] != 0.0f)\n> @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::BRIGHTNESS: {\n> \t\t\tRPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"contrast\"));\n> -\t\t\tASSERT(contrast);\n> +\t\t\tif (!contrast) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set BRIGHTNESS - no contrast algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tcontrast->SetBrightness(ctrl.second.get() * 65536);\n> \t\t\tlibcameraMetadata_.set(controls::Brightness,\n> @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::CONTRAST: {\n> \t\t\tRPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"contrast\"));\n> -\t\t\tASSERT(contrast);\n> +\t\t\tif (!contrast) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set CONTRAST - no contrast algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tcontrast->SetContrast(ctrl.second.get());\n> \t\t\tlibcameraMetadata_.set(controls::Contrast,\n> @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::SATURATION: {\n> \t\t\tRPiController::CcmAlgorithm *ccm = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"ccm\"));\n> -\t\t\tASSERT(ccm);\n> +\t\t\tif (!ccm) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set SATURATION - no ccm algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tccm->SetSaturation(ctrl.second.get());\n> \t\t\tlibcameraMetadata_.set(controls::Saturation,\n> @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\tcase controls::SHARPNESS: {\n> \t\t\tRPiController::SharpenAlgorithm *sharpen = dynamic_cast(\n> \t\t\t\tcontroller_.GetAlgorithm(\"sharpen\"));\n> -\t\t\tASSERT(sharpen);\n> +\t\t\tif (!sharpen) {\n> +\t\t\t\tLOG(IPARPI, Warning)\n> +\t\t\t\t\t<< \"Could not set SHARPNESS - no sharpen algorithm\";\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \n> \t\t\tsharpen->SetStrength(ctrl.second.get());\n> \t\t\tlibcameraMetadata_.set(controls::Sharpness,","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 586E2C0F2B\n\tfor ;\n\tWed, 27 Jan 2021 10:19:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A72B468365;\n\tWed, 27 Jan 2021 11:19:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A3596835F\n\tfor ;\n\tWed, 27 Jan 2021 11:19:04 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8C219240;\n\tWed, 27 Jan 2021 11:19:03 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jnFvfybK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611742743;\n\tbh=1zCjjU4cPO3+UBnSb/M2KavS23biPpm/CqQ0NZbGbgw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jnFvfybKVyc1sfT1ADQ7JSLtcWvtorK0M2B+4gj5nkC2W7x0hzPp6QtQTK26euLyG\n\tqfJoo6k4h95hfClal8oM2T4qnPkO1oA5PnGogn+riwBCd2GdWRRaz1yFqRQW7xYNHD\n\t7YoHXga5tWLbJOxtd0nh//11vGFJEmThgOZ7z9SM=","Date":"Wed, 27 Jan 2021 12:18:44 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210126154724.7155-1-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210126154724.7155-1-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14827,"web_url":"https://patchwork.libcamera.org/comment/14827/","msgid":"","date":"2021-01-27T10:44:19","subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the comments. Let me just add a few more words for the\nsituation I'm thinking of.\n\nOn Wed, 27 Jan 2021 at 10:19, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:\n> > Users are free to avoid loading certain control algorithms from the\n> > json tuning file if they wish. Under such circumstances, failing\n> > completely when an application tries to set parameters for that\n> > algorithm is unhelpful, and it is better just to issue a warning.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----\n> > 1 file changed, 72 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 5ccc7a65..b57d77e9 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > switch (ctrl.first) {\n> > case controls::AE_ENABLE: {\n> > RPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> > - ASSERT(agc);\n> > + if (!agc) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set AE_ENABLE - no AGC algorithm\";\n> > + break;\n> > + }\n>\n> This is a better behaviour indeed. I wonder if we need some kind of\n> LOG_ONCE() to avoid flooding the log, but it would be difficult to do so\n> per-camera.\n>\n> Not aborting when a control is set without a corresponding algorithm is\n> good, but I think we need to go one step further and not report the\n> control to the application in the first place. This will require\n> replacing the static ControlInfoMap in\n> include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We\n> should then extend the libcamera core to verify that all controls in a\n> request are supported and either reject the request or strip the\n> unsupported controls, and we won't need this patch anymore.\n>\n> I'm fine merging this patch as-is, given that the ongoing IPA IPC work\n> should make it easier to handle controls dynamically when it will land,\n> but it should then probably be reverted.\n\nSo the specific thing that led to the change was the following.\n\nNormally I can run a libcamera application of my choice, such as qcam\nor the Raspberry Pi libcamera-hello. Now, I might decide that I don't\nwant the control algorithms to touch (for example) the gamma curve any\nmore. So I comment the \"rpi.contrast\" algorithm out of the json file.\n\nNow, qcam will still run quite happily (I think it sets almost no\ncontrols) but libcamera-hello (prior to this change) would assert and\nfail. The reason is that it tries to set the brightness to the value\nchosen by the user (or its default), and if there's no contrast\nalgorithm, it can't do this. So the idea is that omitting certain\nalgorithm(s) doesn't mean you have to start editing your application\ncode.\n\nHowever, I actually quite like the fact that the warning nags quite a\nlot. It may well be that a single warning would go by during start-up\nand is relatively easy to miss... how about a LOG_N_TIMES version?\n\nThanks!\nDavid\n\n>\n> > +\n> > if (ctrl.second.get() == false)\n> > agc->Pause();\n> > else\n> > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::EXPOSURE_TIME: {\n> > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > controller_.GetAlgorithm(\"agc\"));\n> > - ASSERT(agc);\n> > + if (!agc) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set EXPOSURE_TIME - no AGC algorithm\";\n> > + break;\n> > + }\n> >\n> > /* This expects units of micro-seconds. */\n> > agc->SetFixedShutter(ctrl.second.get());\n> > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::ANALOGUE_GAIN: {\n> > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > controller_.GetAlgorithm(\"agc\"));\n> > - ASSERT(agc);\n> > + if (!agc) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set ANALOGUE_GAIN - no AGC algorithm\";\n> > + break;\n> > + }\n> > +\n> > agc->SetFixedAnalogueGain(ctrl.second.get());\n> >\n> > libcameraMetadata_.set(controls::AnalogueGain,\n> > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::AE_METERING_MODE: {\n> > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > controller_.GetAlgorithm(\"agc\"));\n> > - ASSERT(agc);\n> > + if (!agc) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set AE_METERING_MODE - no AGC algorithm\";\n> > + break;\n> > + }\n> >\n> > int32_t idx = ctrl.second.get();\n> > if (MeteringModeTable.count(idx)) {\n> > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::AE_CONSTRAINT_MODE: {\n> > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > controller_.GetAlgorithm(\"agc\"));\n> > - ASSERT(agc);\n> > + if (!agc) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set AE_CONSTRAINT_MODE - no AGC algorithm\";\n> > + break;\n> > + }\n> >\n> > int32_t idx = ctrl.second.get();\n> > if (ConstraintModeTable.count(idx)) {\n> > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::AE_EXPOSURE_MODE: {\n> > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > controller_.GetAlgorithm(\"agc\"));\n> > - ASSERT(agc);\n> > + if (!agc) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set AE_EXPOSURE_MODE - no AGC algorithm\";\n> > + break;\n> > + }\n> >\n> > int32_t idx = ctrl.second.get();\n> > if (ExposureModeTable.count(idx)) {\n> > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::EXPOSURE_VALUE: {\n> > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > controller_.GetAlgorithm(\"agc\"));\n> > - ASSERT(agc);\n> > + if (!agc) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set EXPOSURE_VALUE - no AGC algorithm\";\n> > + break;\n> > + }\n> >\n> > /*\n> > * The SetEv() method takes in a direct exposure multiplier.\n> > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> >\n> > case controls::AWB_ENABLE: {\n> > RPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> > - ASSERT(awb);\n> > + if (!awb) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set AWB_ENABLE - no AWB algorithm\";\n> > + break;\n> > + }\n> >\n> > if (ctrl.second.get() == false)\n> > awb->Pause();\n> > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::AWB_MODE: {\n> > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > controller_.GetAlgorithm(\"awb\"));\n> > - ASSERT(awb);\n> > + if (!awb) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set AWB_MODE - no AWB algorithm\";\n> > + break;\n> > + }\n> >\n> > int32_t idx = ctrl.second.get();\n> > if (AwbModeTable.count(idx)) {\n> > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > auto gains = ctrl.second.get>();\n> > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > controller_.GetAlgorithm(\"awb\"));\n> > - ASSERT(awb);\n> > + if (!awb) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set COLOUR_GAINS - no AWB algorithm\";\n> > + break;\n> > + }\n> >\n> > awb->SetManualGains(gains[0], gains[1]);\n> > if (gains[0] != 0.0f && gains[1] != 0.0f)\n> > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::BRIGHTNESS: {\n> > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > controller_.GetAlgorithm(\"contrast\"));\n> > - ASSERT(contrast);\n> > + if (!contrast) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set BRIGHTNESS - no contrast algorithm\";\n> > + break;\n> > + }\n> >\n> > contrast->SetBrightness(ctrl.second.get() * 65536);\n> > libcameraMetadata_.set(controls::Brightness,\n> > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::CONTRAST: {\n> > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > controller_.GetAlgorithm(\"contrast\"));\n> > - ASSERT(contrast);\n> > + if (!contrast) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set CONTRAST - no contrast algorithm\";\n> > + break;\n> > + }\n> >\n> > contrast->SetContrast(ctrl.second.get());\n> > libcameraMetadata_.set(controls::Contrast,\n> > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::SATURATION: {\n> > RPiController::CcmAlgorithm *ccm = dynamic_cast(\n> > controller_.GetAlgorithm(\"ccm\"));\n> > - ASSERT(ccm);\n> > + if (!ccm) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set SATURATION - no ccm algorithm\";\n> > + break;\n> > + }\n> >\n> > ccm->SetSaturation(ctrl.second.get());\n> > libcameraMetadata_.set(controls::Saturation,\n> > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > case controls::SHARPNESS: {\n> > RPiController::SharpenAlgorithm *sharpen = dynamic_cast(\n> > controller_.GetAlgorithm(\"sharpen\"));\n> > - ASSERT(sharpen);\n> > + if (!sharpen) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set SHARPNESS - no sharpen algorithm\";\n> > + break;\n> > + }\n> >\n> > sharpen->SetStrength(ctrl.second.get());\n> > libcameraMetadata_.set(controls::Sharpness,\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 72B4DBD808\n\tfor ;\n\tWed, 27 Jan 2021 10:44:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 02C9D68364;\n\tWed, 27 Jan 2021 11:44:34 +0100 (CET)","from mail-oi1-x22c.google.com (mail-oi1-x22c.google.com\n\t[IPv6:2607:f8b0:4864:20::22c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9CD2168364\n\tfor ;\n\tWed, 27 Jan 2021 11:44:31 +0100 (CET)","by mail-oi1-x22c.google.com with SMTP id a77so1678257oii.4\n\tfor ;\n\tWed, 27 Jan 2021 02:44:31 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"loEXa1+4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=dlSZtTjudHrhJH998DXN7kvXFNiNaTff2C1FAY5K+CY=;\n\tb=loEXa1+4GoxvnVsXaL0HinFt7GStQDI+bneIoC49dz6OZCXsiXYIZon2zoYcSg7z/s\n\ttA6ZF+RDwmIH2tBhGRNixQrSWkpQnQ21vGzgtAiX53VOdZKG0R2LSQHUTh5+GwMsJeQu\n\tzk93cdpAZy/mY1icu5niXSOrRIYhOqkHN0HpehjmISF/oexmAQZPyo76SifCFovGzfPE\n\t7dj3dgT9Vr8kfv1BTiOuYFtdhGyRS+vLIkADzy9u84NNyivAgaOC68c4HDTtrnTxq6ZH\n\t4O0fDAYoxxZbABOv4Qi0TlqD6pm+4Iy5P6NbZ3iegufMz6J/9m0SVbbanaWn3yOR0ZzL\n\tPGWg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=dlSZtTjudHrhJH998DXN7kvXFNiNaTff2C1FAY5K+CY=;\n\tb=Lr4lFIusozS+MfYAwYjn0GwrZaGT5pJlbHh8Bb9vkI2CsL2CE3MhuPmtSDxJNMk/sB\n\tyeTBPq07Bhbpcc5NEUPT7jIG7xJujgkH8B2xk6t4tPCVS8Eu/oONnAdgyrsM7fcbjTrJ\n\t/k1DPYL75CP3Y5ZZVapwzAgXzhLIEx/ZWzS2RR/jYbLOUY4j4FKMY2T/1BC40yCLmKY+\n\tTY++tVWHbY9VlzSJWmrubosPj1qR6KS+oIhHRFhYU2bh89niGZUCDSsPsTkN6gNvvMtg\n\tUzW1uzZSZprUzgh5t+AJrgA0GaxsBxYetSwUPYvjYtcpzIOVae8rYyHItkqEjmBhgGh6\n\t3Phg==","X-Gm-Message-State":"AOAM533t8BfvpCIrM3MgeEUqgRP52eIzNDBfhHI2+y8LfE1HjF6rIufT\n\tBoONW97E9cdTpcEnP53HJpieRE+x1tbZ4zX2gOsD40ZgzjblgA==","X-Google-Smtp-Source":"ABdhPJxLsE2UL8RdFVC3gsm/p/Pgulp2mwuukZdzwsW9mBETuYm5+kdUJ7rr+b0F4DVslMUBhQMTXABDjt2wITQZ8zs=","X-Received":"by 2002:a05:6808:8cb:: with SMTP id\n\tk11mr2669479oij.22.1611744270240; \n\tWed, 27 Jan 2021 02:44:30 -0800 (PST)","MIME-Version":"1.0","References":"<20210126154724.7155-1-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 27 Jan 2021 10:44:19 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14828,"web_url":"https://patchwork.libcamera.org/comment/14828/","msgid":"","date":"2021-01-27T10:59:52","subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"Hi David\n\nOn Wed, 27 Jan 2021 at 10:44, David Plowman\n wrote:\n>\n> Hi Laurent\n>\n> Thanks for the comments. Let me just add a few more words for the\n> situation I'm thinking of.\n>\n> On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart\n> wrote:\n> >\n> > Hi David,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:\n> > > Users are free to avoid loading certain control algorithms from the\n> > > json tuning file if they wish. Under such circumstances, failing\n> > > completely when an application tries to set parameters for that\n> > > algorithm is unhelpful, and it is better just to issue a warning.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----\n> > > 1 file changed, 72 insertions(+), 14 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 5ccc7a65..b57d77e9 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > switch (ctrl.first) {\n> > > case controls::AE_ENABLE: {\n> > > RPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AE_ENABLE - no AGC algorithm\";\n> > > + break;\n> > > + }\n> >\n> > This is a better behaviour indeed. I wonder if we need some kind of\n> > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so\n> > per-camera.\n> >\n> > Not aborting when a control is set without a corresponding algorithm is\n> > good, but I think we need to go one step further and not report the\n> > control to the application in the first place. This will require\n> > replacing the static ControlInfoMap in\n> > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We\n> > should then extend the libcamera core to verify that all controls in a\n> > request are supported and either reject the request or strip the\n> > unsupported controls, and we won't need this patch anymore.\n> >\n> > I'm fine merging this patch as-is, given that the ongoing IPA IPC work\n> > should make it easier to handle controls dynamically when it will land,\n> > but it should then probably be reverted.\n>\n> So the specific thing that led to the change was the following.\n>\n> Normally I can run a libcamera application of my choice, such as qcam\n> or the Raspberry Pi libcamera-hello. Now, I might decide that I don't\n> want the control algorithms to touch (for example) the gamma curve any\n> more. So I comment the \"rpi.contrast\" algorithm out of the json file.\n>\n> Now, qcam will still run quite happily (I think it sets almost no\n> controls) but libcamera-hello (prior to this change) would assert and\n> fail. The reason is that it tries to set the brightness to the value\n> chosen by the user (or its default), and if there's no contrast\n> algorithm, it can't do this. So the idea is that omitting certain\n> algorithm(s) doesn't mean you have to start editing your application\n> code.\n>\n> However, I actually quite like the fact that the warning nags quite a\n> lot. It may well be that a single warning would go by during start-up\n> and is relatively easy to miss... how about a LOG_N_TIMES version?\n\nUnless I've misunderstood, won't this also apply for monochrome\nsensors where you won't want AWB to run as there is no colour? Logging\nany form of error or warning there is really unwanted.\n\nI have an increasing pile of mono sensors with working drivers that I\ncan pass on if you wanted to try it out! (OV9281, OV7251, and OV2311\nfor starters).\n\n Dave\n\n> Thanks!\n> David\n>\n> >\n> > > +\n> > > if (ctrl.second.get() == false)\n> > > agc->Pause();\n> > > else\n> > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::EXPOSURE_TIME: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set EXPOSURE_TIME - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > /* This expects units of micro-seconds. */\n> > > agc->SetFixedShutter(ctrl.second.get());\n> > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::ANALOGUE_GAIN: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set ANALOGUE_GAIN - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > > +\n> > > agc->SetFixedAnalogueGain(ctrl.second.get());\n> > >\n> > > libcameraMetadata_.set(controls::AnalogueGain,\n> > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::AE_METERING_MODE: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AE_METERING_MODE - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > int32_t idx = ctrl.second.get();\n> > > if (MeteringModeTable.count(idx)) {\n> > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::AE_CONSTRAINT_MODE: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AE_CONSTRAINT_MODE - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > int32_t idx = ctrl.second.get();\n> > > if (ConstraintModeTable.count(idx)) {\n> > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::AE_EXPOSURE_MODE: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AE_EXPOSURE_MODE - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > int32_t idx = ctrl.second.get();\n> > > if (ExposureModeTable.count(idx)) {\n> > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::EXPOSURE_VALUE: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set EXPOSURE_VALUE - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > /*\n> > > * The SetEv() method takes in a direct exposure multiplier.\n> > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > >\n> > > case controls::AWB_ENABLE: {\n> > > RPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> > > - ASSERT(awb);\n> > > + if (!awb) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AWB_ENABLE - no AWB algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > if (ctrl.second.get() == false)\n> > > awb->Pause();\n> > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::AWB_MODE: {\n> > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > controller_.GetAlgorithm(\"awb\"));\n> > > - ASSERT(awb);\n> > > + if (!awb) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AWB_MODE - no AWB algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > int32_t idx = ctrl.second.get();\n> > > if (AwbModeTable.count(idx)) {\n> > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > auto gains = ctrl.second.get>();\n> > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > controller_.GetAlgorithm(\"awb\"));\n> > > - ASSERT(awb);\n> > > + if (!awb) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set COLOUR_GAINS - no AWB algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > awb->SetManualGains(gains[0], gains[1]);\n> > > if (gains[0] != 0.0f && gains[1] != 0.0f)\n> > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::BRIGHTNESS: {\n> > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > controller_.GetAlgorithm(\"contrast\"));\n> > > - ASSERT(contrast);\n> > > + if (!contrast) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set BRIGHTNESS - no contrast algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > contrast->SetBrightness(ctrl.second.get() * 65536);\n> > > libcameraMetadata_.set(controls::Brightness,\n> > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::CONTRAST: {\n> > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > controller_.GetAlgorithm(\"contrast\"));\n> > > - ASSERT(contrast);\n> > > + if (!contrast) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set CONTRAST - no contrast algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > contrast->SetContrast(ctrl.second.get());\n> > > libcameraMetadata_.set(controls::Contrast,\n> > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::SATURATION: {\n> > > RPiController::CcmAlgorithm *ccm = dynamic_cast(\n> > > controller_.GetAlgorithm(\"ccm\"));\n> > > - ASSERT(ccm);\n> > > + if (!ccm) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set SATURATION - no ccm algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > ccm->SetSaturation(ctrl.second.get());\n> > > libcameraMetadata_.set(controls::Saturation,\n> > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::SHARPNESS: {\n> > > RPiController::SharpenAlgorithm *sharpen = dynamic_cast(\n> > > controller_.GetAlgorithm(\"sharpen\"));\n> > > - ASSERT(sharpen);\n> > > + if (!sharpen) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set SHARPNESS - no sharpen algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > sharpen->SetStrength(ctrl.second.get());\n> > > libcameraMetadata_.set(controls::Sharpness,\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 EB050C0F2B\n\tfor ;\n\tWed, 27 Jan 2021 11:00:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B15AD6836E;\n\tWed, 27 Jan 2021 12:00:09 +0100 (CET)","from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com\n\t[IPv6:2a00:1450:4864:20::42e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37404682D4\n\tfor ;\n\tWed, 27 Jan 2021 12:00:08 +0100 (CET)","by mail-wr1-x42e.google.com with SMTP id a1so1428277wrq.6\n\tfor ;\n\tWed, 27 Jan 2021 03:00:08 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"bNoZeVKY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=WIDuiv9tt0LoG3lsuDY22LpvLPBoALO/Pl1zUrh2YWw=;\n\tb=bNoZeVKY831shlCIOH+J8NuF+biV7wC6k2tt3OQPQPEhTsxnbFjDo+oE7z+4QfsV11\n\tfxxBN5ms5xWP/94rGBJLO8dGjban+17Qf7+h7xchgh7egQsssi6brvwxPRe0zMUF5m2c\n\tJYByCjI1jwqV/jmyVvzf5AW4o93zbWCkClXhsbBO86CdWlr8mZkVLm1I7X9bgnX4CAdn\n\tonJMeQlzhV7x8BjmyRDnBLmpYLZLTG01G2isrXL0354NKRZRGjnBo7JTSi7RZlyEsmnv\n\tcAzqrpEIJuMYbXdDSB3UYW5AXY2E7VcEgfYwrIa9xnbDOnDuT0dQqyhSxzz69sReGJDA\n\t2C2w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=WIDuiv9tt0LoG3lsuDY22LpvLPBoALO/Pl1zUrh2YWw=;\n\tb=qXLAvBbutl2kEEEoYqDJhcHmACF0oRZil2fcTlTYTQbPTfbCGwpTTbtxLbLfdJUcww\n\tudMRcv1tiXABwtwwRsec5Y9sjDc02HblFsw2pC0zZmSJZqqGfZ6oFXxRPLa67wNxeKwn\n\tBd1vTa8SeTRQ+OYCdwqBleoMhLbtaIHgkDnIWCE3thj/fSiQToykbt0k41aW7nyVXX5i\n\tOJWCOMUSdo3acbH2czQye3POtLvdTqdkn4bMrMcKu6QZPBbXfG0sXt8mhbkZ24GiyvYV\n\ttlGY5H/711DbTZNlS+M8qmjsDpPy+80cAw5lGJ18Kva07S0L+aW19rvtr/d7iY/Z8Lbq\n\t9haA==","X-Gm-Message-State":"AOAM533RIEBSHkLTJwCRdxEg3C8AD3nRhI6Ql5rmP1b5Zg08BRv1huQ/\n\tDoiJk+Jbe3zUdS9L7o325TjdwLy7TW/s7rBKBT5ErA==","X-Google-Smtp-Source":"ABdhPJxnxrkbe7oFSMBZTVLcVrkoPSnjMU5eYcrvh0fYjWrMGfhZ3iQMSRHMrJ8+I5WBEPefQbIyDL29bv0awEa/O/w=","X-Received":"by 2002:a5d:4882:: with SMTP id\n\tg2mr10415278wrq.273.1611745207812; \n\tWed, 27 Jan 2021 03:00:07 -0800 (PST)","MIME-Version":"1.0","References":"<20210126154724.7155-1-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Dave Stevenson ","Date":"Wed, 27 Jan 2021 10:59:52 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14829,"web_url":"https://patchwork.libcamera.org/comment/14829/","msgid":"","date":"2021-01-27T11:17:37","subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Wed, Jan 27, 2021 at 10:44:19AM +0000, David Plowman wrote:\n> Hi Laurent\n> \n> Thanks for the comments. Let me just add a few more words for the\n> situation I'm thinking of.\n> \n> On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart wrote:\n> > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:\n> > > Users are free to avoid loading certain control algorithms from the\n> > > json tuning file if they wish. Under such circumstances, failing\n> > > completely when an application tries to set parameters for that\n> > > algorithm is unhelpful, and it is better just to issue a warning.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----\n> > > 1 file changed, 72 insertions(+), 14 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 5ccc7a65..b57d77e9 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > switch (ctrl.first) {\n> > > case controls::AE_ENABLE: {\n> > > RPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AE_ENABLE - no AGC algorithm\";\n> > > + break;\n> > > + }\n> >\n> > This is a better behaviour indeed. I wonder if we need some kind of\n> > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so\n> > per-camera.\n> >\n> > Not aborting when a control is set without a corresponding algorithm is\n> > good, but I think we need to go one step further and not report the\n> > control to the application in the first place. This will require\n> > replacing the static ControlInfoMap in\n> > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We\n> > should then extend the libcamera core to verify that all controls in a\n> > request are supported and either reject the request or strip the\n> > unsupported controls, and we won't need this patch anymore.\n> >\n> > I'm fine merging this patch as-is, given that the ongoing IPA IPC work\n> > should make it easier to handle controls dynamically when it will land,\n> > but it should then probably be reverted.\n> \n> So the specific thing that led to the change was the following.\n> \n> Normally I can run a libcamera application of my choice, such as qcam\n> or the Raspberry Pi libcamera-hello. Now, I might decide that I don't\n> want the control algorithms to touch (for example) the gamma curve any\n> more. So I comment the \"rpi.contrast\" algorithm out of the json file.\n> \n> Now, qcam will still run quite happily (I think it sets almost no\n> controls) but libcamera-hello (prior to this change) would assert and\n> fail. The reason is that it tries to set the brightness to the value\n> chosen by the user (or its default), and if there's no contrast\n> algorithm, it can't do this. So the idea is that omitting certain\n> algorithm(s) doesn't mean you have to start editing your application\n> code.\n\nThe API concept is that applications should only set controls that are\nreported by the camera as supported. That puts a bit of an extra burden\non the application, but for applications that don't care about controls\nbeing ignored if unsupported, it would be easy to implement a small\ncontrol set wrapper function that would skip controls that are not\nadvertised.\n\n> However, I actually quite like the fact that the warning nags quite a\n> lot. It may well be that a single warning would go by during start-up\n> and is relatively easy to miss... how about a LOG_N_TIMES version?\n> \n> > > +\n> > > if (ctrl.second.get() == false)\n> > > agc->Pause();\n> > > else\n> > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::EXPOSURE_TIME: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set EXPOSURE_TIME - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > /* This expects units of micro-seconds. */\n> > > agc->SetFixedShutter(ctrl.second.get());\n> > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::ANALOGUE_GAIN: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set ANALOGUE_GAIN - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > > +\n> > > agc->SetFixedAnalogueGain(ctrl.second.get());\n> > >\n> > > libcameraMetadata_.set(controls::AnalogueGain,\n> > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::AE_METERING_MODE: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AE_METERING_MODE - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > int32_t idx = ctrl.second.get();\n> > > if (MeteringModeTable.count(idx)) {\n> > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::AE_CONSTRAINT_MODE: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AE_CONSTRAINT_MODE - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > int32_t idx = ctrl.second.get();\n> > > if (ConstraintModeTable.count(idx)) {\n> > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::AE_EXPOSURE_MODE: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AE_EXPOSURE_MODE - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > int32_t idx = ctrl.second.get();\n> > > if (ExposureModeTable.count(idx)) {\n> > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::EXPOSURE_VALUE: {\n> > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > controller_.GetAlgorithm(\"agc\"));\n> > > - ASSERT(agc);\n> > > + if (!agc) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set EXPOSURE_VALUE - no AGC algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > /*\n> > > * The SetEv() method takes in a direct exposure multiplier.\n> > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > >\n> > > case controls::AWB_ENABLE: {\n> > > RPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> > > - ASSERT(awb);\n> > > + if (!awb) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AWB_ENABLE - no AWB algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > if (ctrl.second.get() == false)\n> > > awb->Pause();\n> > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::AWB_MODE: {\n> > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > controller_.GetAlgorithm(\"awb\"));\n> > > - ASSERT(awb);\n> > > + if (!awb) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set AWB_MODE - no AWB algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > int32_t idx = ctrl.second.get();\n> > > if (AwbModeTable.count(idx)) {\n> > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > auto gains = ctrl.second.get>();\n> > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > controller_.GetAlgorithm(\"awb\"));\n> > > - ASSERT(awb);\n> > > + if (!awb) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set COLOUR_GAINS - no AWB algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > awb->SetManualGains(gains[0], gains[1]);\n> > > if (gains[0] != 0.0f && gains[1] != 0.0f)\n> > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::BRIGHTNESS: {\n> > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > controller_.GetAlgorithm(\"contrast\"));\n> > > - ASSERT(contrast);\n> > > + if (!contrast) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set BRIGHTNESS - no contrast algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > contrast->SetBrightness(ctrl.second.get() * 65536);\n> > > libcameraMetadata_.set(controls::Brightness,\n> > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::CONTRAST: {\n> > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > controller_.GetAlgorithm(\"contrast\"));\n> > > - ASSERT(contrast);\n> > > + if (!contrast) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set CONTRAST - no contrast algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > contrast->SetContrast(ctrl.second.get());\n> > > libcameraMetadata_.set(controls::Contrast,\n> > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::SATURATION: {\n> > > RPiController::CcmAlgorithm *ccm = dynamic_cast(\n> > > controller_.GetAlgorithm(\"ccm\"));\n> > > - ASSERT(ccm);\n> > > + if (!ccm) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set SATURATION - no ccm algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > ccm->SetSaturation(ctrl.second.get());\n> > > libcameraMetadata_.set(controls::Saturation,\n> > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > case controls::SHARPNESS: {\n> > > RPiController::SharpenAlgorithm *sharpen = dynamic_cast(\n> > > controller_.GetAlgorithm(\"sharpen\"));\n> > > - ASSERT(sharpen);\n> > > + if (!sharpen) {\n> > > + LOG(IPARPI, Warning)\n> > > + << \"Could not set SHARPNESS - no sharpen algorithm\";\n> > > + break;\n> > > + }\n> > >\n> > > sharpen->SetStrength(ctrl.second.get());\n> > > libcameraMetadata_.set(controls::Sharpness,","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 EB037BD808\n\tfor ;\n\tWed, 27 Jan 2021 11:17:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BF1360F30;\n\tWed, 27 Jan 2021 12:17:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B6ED660F1F\n\tfor ;\n\tWed, 27 Jan 2021 12:17:57 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2B47C240;\n\tWed, 27 Jan 2021 12:17:57 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TAgZK95J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611746277;\n\tbh=5ec97yO/Ln6mrzDujRcwrMgTxmWWciyoEdJYAszmZwE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TAgZK95J8RmY4qQfQ60jPuXLZz/mrT1qJG4uJNJnbzRMTBFfrhjG0ji6rF81yf5gl\n\tJ/Bfxt26tUb0JaLoVAgEJEE66XGNVNNu9FAOCgQfM5bLdNCSbg85cypgPojK4L6hy0\n\tyo0y1YWKP4DFceuUTrVDmHNf09lXTXnjFqdNnHkA=","Date":"Wed, 27 Jan 2021 13:17:37 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210126154724.7155-1-david.plowman@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14830,"web_url":"https://patchwork.libcamera.org/comment/14830/","msgid":"","date":"2021-01-27T11:33:28","subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Dave\n\nYes, I'm a bit in several minds over monochrome sensors. Should\nalgorithms know whether sensors are colour or monochrome? Should\ncertain algorithms just be deleted from the tuning file? Probably, but\nit does want some thinking about.\n\nAs regards this patch, I don't think anything gets worse (actually you\ncan delete the AWB and CCM algorithms and your camera will still run).\nThere should be just one grumble at startup when it tries to set the\nAWB mode. AGC is actually more annoying, though it's not affected\neither way by this patch, as it complains repeatedly if it has no AWB\nmetadata. Either the algorithm would have to know not to care, or\nperhaps that warning isn't really very useful these days and could be\ndemoted.\n\nCertainly someone (e.g. me) needs to spend some time with these\nmonochrome sensors, calibrating them and making sure everything works.\nThe tuning tool will have issues with them too, but it would be great\nto have more supported sensors. Do they all have fully functional V4L2\ndrivers for libcamera?\n\nThanks!\nDavid\n\nOn Wed, 27 Jan 2021 at 11:00, Dave Stevenson\n wrote:\n>\n> Hi David\n>\n> On Wed, 27 Jan 2021 at 10:44, David Plowman\n> wrote:\n> >\n> > Hi Laurent\n> >\n> > Thanks for the comments. Let me just add a few more words for the\n> > situation I'm thinking of.\n> >\n> > On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart\n> > wrote:\n> > >\n> > > Hi David,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:\n> > > > Users are free to avoid loading certain control algorithms from the\n> > > > json tuning file if they wish. Under such circumstances, failing\n> > > > completely when an application tries to set parameters for that\n> > > > algorithm is unhelpful, and it is better just to issue a warning.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----\n> > > > 1 file changed, 72 insertions(+), 14 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 5ccc7a65..b57d77e9 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > switch (ctrl.first) {\n> > > > case controls::AE_ENABLE: {\n> > > > RPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AE_ENABLE - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > >\n> > > This is a better behaviour indeed. I wonder if we need some kind of\n> > > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so\n> > > per-camera.\n> > >\n> > > Not aborting when a control is set without a corresponding algorithm is\n> > > good, but I think we need to go one step further and not report the\n> > > control to the application in the first place. This will require\n> > > replacing the static ControlInfoMap in\n> > > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We\n> > > should then extend the libcamera core to verify that all controls in a\n> > > request are supported and either reject the request or strip the\n> > > unsupported controls, and we won't need this patch anymore.\n> > >\n> > > I'm fine merging this patch as-is, given that the ongoing IPA IPC work\n> > > should make it easier to handle controls dynamically when it will land,\n> > > but it should then probably be reverted.\n> >\n> > So the specific thing that led to the change was the following.\n> >\n> > Normally I can run a libcamera application of my choice, such as qcam\n> > or the Raspberry Pi libcamera-hello. Now, I might decide that I don't\n> > want the control algorithms to touch (for example) the gamma curve any\n> > more. So I comment the \"rpi.contrast\" algorithm out of the json file.\n> >\n> > Now, qcam will still run quite happily (I think it sets almost no\n> > controls) but libcamera-hello (prior to this change) would assert and\n> > fail. The reason is that it tries to set the brightness to the value\n> > chosen by the user (or its default), and if there's no contrast\n> > algorithm, it can't do this. So the idea is that omitting certain\n> > algorithm(s) doesn't mean you have to start editing your application\n> > code.\n> >\n> > However, I actually quite like the fact that the warning nags quite a\n> > lot. It may well be that a single warning would go by during start-up\n> > and is relatively easy to miss... how about a LOG_N_TIMES version?\n>\n> Unless I've misunderstood, won't this also apply for monochrome\n> sensors where you won't want AWB to run as there is no colour? Logging\n> any form of error or warning there is really unwanted.\n>\n> I have an increasing pile of mono sensors with working drivers that I\n> can pass on if you wanted to try it out! (OV9281, OV7251, and OV2311\n> for starters).\n>\n> Dave\n>\n> > Thanks!\n> > David\n> >\n> > >\n> > > > +\n> > > > if (ctrl.second.get() == false)\n> > > > agc->Pause();\n> > > > else\n> > > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::EXPOSURE_TIME: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set EXPOSURE_TIME - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > /* This expects units of micro-seconds. */\n> > > > agc->SetFixedShutter(ctrl.second.get());\n> > > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::ANALOGUE_GAIN: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set ANALOGUE_GAIN - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > agc->SetFixedAnalogueGain(ctrl.second.get());\n> > > >\n> > > > libcameraMetadata_.set(controls::AnalogueGain,\n> > > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::AE_METERING_MODE: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AE_METERING_MODE - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > int32_t idx = ctrl.second.get();\n> > > > if (MeteringModeTable.count(idx)) {\n> > > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::AE_CONSTRAINT_MODE: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AE_CONSTRAINT_MODE - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > int32_t idx = ctrl.second.get();\n> > > > if (ConstraintModeTable.count(idx)) {\n> > > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::AE_EXPOSURE_MODE: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AE_EXPOSURE_MODE - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > int32_t idx = ctrl.second.get();\n> > > > if (ExposureModeTable.count(idx)) {\n> > > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::EXPOSURE_VALUE: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set EXPOSURE_VALUE - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > /*\n> > > > * The SetEv() method takes in a direct exposure multiplier.\n> > > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > >\n> > > > case controls::AWB_ENABLE: {\n> > > > RPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> > > > - ASSERT(awb);\n> > > > + if (!awb) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AWB_ENABLE - no AWB algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > if (ctrl.second.get() == false)\n> > > > awb->Pause();\n> > > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::AWB_MODE: {\n> > > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"awb\"));\n> > > > - ASSERT(awb);\n> > > > + if (!awb) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AWB_MODE - no AWB algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > int32_t idx = ctrl.second.get();\n> > > > if (AwbModeTable.count(idx)) {\n> > > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > auto gains = ctrl.second.get>();\n> > > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"awb\"));\n> > > > - ASSERT(awb);\n> > > > + if (!awb) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set COLOUR_GAINS - no AWB algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > awb->SetManualGains(gains[0], gains[1]);\n> > > > if (gains[0] != 0.0f && gains[1] != 0.0f)\n> > > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::BRIGHTNESS: {\n> > > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"contrast\"));\n> > > > - ASSERT(contrast);\n> > > > + if (!contrast) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set BRIGHTNESS - no contrast algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > contrast->SetBrightness(ctrl.second.get() * 65536);\n> > > > libcameraMetadata_.set(controls::Brightness,\n> > > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::CONTRAST: {\n> > > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"contrast\"));\n> > > > - ASSERT(contrast);\n> > > > + if (!contrast) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set CONTRAST - no contrast algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > contrast->SetContrast(ctrl.second.get());\n> > > > libcameraMetadata_.set(controls::Contrast,\n> > > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::SATURATION: {\n> > > > RPiController::CcmAlgorithm *ccm = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"ccm\"));\n> > > > - ASSERT(ccm);\n> > > > + if (!ccm) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set SATURATION - no ccm algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > ccm->SetSaturation(ctrl.second.get());\n> > > > libcameraMetadata_.set(controls::Saturation,\n> > > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::SHARPNESS: {\n> > > > RPiController::SharpenAlgorithm *sharpen = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"sharpen\"));\n> > > > - ASSERT(sharpen);\n> > > > + if (!sharpen) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set SHARPNESS - no sharpen algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > sharpen->SetStrength(ctrl.second.get());\n> > > > libcameraMetadata_.set(controls::Sharpness,\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 8B742C0F2B\n\tfor ;\n\tWed, 27 Jan 2021 11:33:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DB6560F2F;\n\tWed, 27 Jan 2021 12:33:41 +0100 (CET)","from mail-ot1-x32c.google.com (mail-ot1-x32c.google.com\n\t[IPv6:2607:f8b0:4864:20::32c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E705560F1F\n\tfor ;\n\tWed, 27 Jan 2021 12:33:39 +0100 (CET)","by mail-ot1-x32c.google.com with SMTP id s2so1333823otp.5\n\tfor ;\n\tWed, 27 Jan 2021 03:33:39 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"FYDXPs7M\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=6y9UgLn1ksMAX6GL/+mLlFTuzqJBYrndhu4tc5SDqR4=;\n\tb=FYDXPs7Mf2rzVUeHjQ7FFJ1spkgs9uyyFa80mmvEL5TRF1eRLpryBZqkuPao6glpB2\n\tQIDKff4ITVQd2lQsXGi4Kt7iu7DZ8+fQHocVVkf8VoSI6d4q9Kt8ZOTZBKFbmUylcHn9\n\tGSRGfBgmrKPDVM6IiS9JYJJUZLZEs8BlqeSlpaqIxp0//N/O64iKmLT028wmEs8bCMyP\n\tiCmBtskiaBSdL/KoeYv1tqIFYxm1fs29o8H/HD2JWUw+cs8Xx5KEOv3qpyPfOCcsuV/L\n\tojPol5SLgDx2RprKjPROCdV58J9a7aAw9N9nuFpwIYXo1xsKqVd2XT/MoCDkF20E3/Ui\n\tqc6Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=6y9UgLn1ksMAX6GL/+mLlFTuzqJBYrndhu4tc5SDqR4=;\n\tb=b686ME/5QCA3aYdg8oSerhUvusEnHS44BbtUOwTPR/kjonOe+IzuR3dSV0tB59b6um\n\tOZiAEkewE7w5esEUYONwQbpccTeZfCwCwODZz+42/wRgFAMLOYlBEkcvxOFy8DzYBxwc\n\tXoQIEDhzRAb3lUb1H1ebwlWrP9LY26MraJIHolSihsDHrnfLNFBBf4Hns4dCE2r6+rAM\n\tg5ZyOsyKBBQCI03s0jXA8jNQWTO0kjkN6fCatvPeliIwEIxOuoLZzGBW5la3USSv1yWP\n\tYYxiPxu1wm0rgPeSPYsXMRdC11nW3/A1vurhL+Z4XpjlM/gJdUzDqFoIcMCxTJ0Eptwf\n\tGu0A==","X-Gm-Message-State":"AOAM531OqwJzmYf/S2cyVifvG5a2fMyzJglxFbGCWY5D0CTrBWoY5K7s\n\tnUIY31inZHx2EK4HM6UZhR6BdoTTSlgYPHvmIAfdGA==","X-Google-Smtp-Source":"ABdhPJz54qfW7JlKKBmnsKfxpVAG6ewF7la+MmZxR+MxvDQFYQ5yxGp8gylUNAaM8c8T80654UroKVhj6ypCsaiW3wo=","X-Received":"by 2002:a05:6830:1e7b:: with SMTP id\n\tm27mr6877275otr.317.1611747218213; \n\tWed, 27 Jan 2021 03:33:38 -0800 (PST)","MIME-Version":"1.0","References":"<20210126154724.7155-1-david.plowman@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 27 Jan 2021 11:33:28 +0000","Message-ID":"","To":"Dave Stevenson ","Subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14833,"web_url":"https://patchwork.libcamera.org/comment/14833/","msgid":"","date":"2021-01-27T11:43:34","subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nOn Wed, 27 Jan 2021 at 11:17, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> On Wed, Jan 27, 2021 at 10:44:19AM +0000, David Plowman wrote:\n> > Hi Laurent\n> >\n> > Thanks for the comments. Let me just add a few more words for the\n> > situation I'm thinking of.\n> >\n> > On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart wrote:\n> > > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:\n> > > > Users are free to avoid loading certain control algorithms from the\n> > > > json tuning file if they wish. Under such circumstances, failing\n> > > > completely when an application tries to set parameters for that\n> > > > algorithm is unhelpful, and it is better just to issue a warning.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----\n> > > > 1 file changed, 72 insertions(+), 14 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 5ccc7a65..b57d77e9 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > switch (ctrl.first) {\n> > > > case controls::AE_ENABLE: {\n> > > > RPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AE_ENABLE - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > >\n> > > This is a better behaviour indeed. I wonder if we need some kind of\n> > > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so\n> > > per-camera.\n> > >\n> > > Not aborting when a control is set without a corresponding algorithm is\n> > > good, but I think we need to go one step further and not report the\n> > > control to the application in the first place. This will require\n> > > replacing the static ControlInfoMap in\n> > > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We\n> > > should then extend the libcamera core to verify that all controls in a\n> > > request are supported and either reject the request or strip the\n> > > unsupported controls, and we won't need this patch anymore.\n> > >\n> > > I'm fine merging this patch as-is, given that the ongoing IPA IPC work\n> > > should make it easier to handle controls dynamically when it will land,\n> > > but it should then probably be reverted.\n> >\n> > So the specific thing that led to the change was the following.\n> >\n> > Normally I can run a libcamera application of my choice, such as qcam\n> > or the Raspberry Pi libcamera-hello. Now, I might decide that I don't\n> > want the control algorithms to touch (for example) the gamma curve any\n> > more. So I comment the \"rpi.contrast\" algorithm out of the json file.\n> >\n> > Now, qcam will still run quite happily (I think it sets almost no\n> > controls) but libcamera-hello (prior to this change) would assert and\n> > fail. The reason is that it tries to set the brightness to the value\n> > chosen by the user (or its default), and if there's no contrast\n> > algorithm, it can't do this. So the idea is that omitting certain\n> > algorithm(s) doesn't mean you have to start editing your application\n> > code.\n>\n> The API concept is that applications should only set controls that are\n> reported by the camera as supported. That puts a bit of an extra burden\n> on the application, but for applications that don't care about controls\n> being ignored if unsupported, it would be easy to implement a small\n> control set wrapper function that would skip controls that are not\n> advertised.\n\nYes, I understand you now. We need \"dynamic controls\" which will\ndepend in our case probably on the control algorithms that have been\nloaded. Checking whether controls are actually supported is perhaps a\nlittle tiresome, but I don't particularly object to our applications\ndoing that.\n\nThanks for the clarifications!\nDavd\n\n>\n> > However, I actually quite like the fact that the warning nags quite a\n> > lot. It may well be that a single warning would go by during start-up\n> > and is relatively easy to miss... how about a LOG_N_TIMES version?\n> >\n> > > > +\n> > > > if (ctrl.second.get() == false)\n> > > > agc->Pause();\n> > > > else\n> > > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::EXPOSURE_TIME: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set EXPOSURE_TIME - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > /* This expects units of micro-seconds. */\n> > > > agc->SetFixedShutter(ctrl.second.get());\n> > > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::ANALOGUE_GAIN: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set ANALOGUE_GAIN - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > > +\n> > > > agc->SetFixedAnalogueGain(ctrl.second.get());\n> > > >\n> > > > libcameraMetadata_.set(controls::AnalogueGain,\n> > > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::AE_METERING_MODE: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AE_METERING_MODE - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > int32_t idx = ctrl.second.get();\n> > > > if (MeteringModeTable.count(idx)) {\n> > > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::AE_CONSTRAINT_MODE: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AE_CONSTRAINT_MODE - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > int32_t idx = ctrl.second.get();\n> > > > if (ConstraintModeTable.count(idx)) {\n> > > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::AE_EXPOSURE_MODE: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AE_EXPOSURE_MODE - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > int32_t idx = ctrl.second.get();\n> > > > if (ExposureModeTable.count(idx)) {\n> > > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::EXPOSURE_VALUE: {\n> > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"agc\"));\n> > > > - ASSERT(agc);\n> > > > + if (!agc) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set EXPOSURE_VALUE - no AGC algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > /*\n> > > > * The SetEv() method takes in a direct exposure multiplier.\n> > > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > >\n> > > > case controls::AWB_ENABLE: {\n> > > > RPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> > > > - ASSERT(awb);\n> > > > + if (!awb) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AWB_ENABLE - no AWB algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > if (ctrl.second.get() == false)\n> > > > awb->Pause();\n> > > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::AWB_MODE: {\n> > > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"awb\"));\n> > > > - ASSERT(awb);\n> > > > + if (!awb) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set AWB_MODE - no AWB algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > int32_t idx = ctrl.second.get();\n> > > > if (AwbModeTable.count(idx)) {\n> > > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > auto gains = ctrl.second.get>();\n> > > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"awb\"));\n> > > > - ASSERT(awb);\n> > > > + if (!awb) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set COLOUR_GAINS - no AWB algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > awb->SetManualGains(gains[0], gains[1]);\n> > > > if (gains[0] != 0.0f && gains[1] != 0.0f)\n> > > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::BRIGHTNESS: {\n> > > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"contrast\"));\n> > > > - ASSERT(contrast);\n> > > > + if (!contrast) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set BRIGHTNESS - no contrast algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > contrast->SetBrightness(ctrl.second.get() * 65536);\n> > > > libcameraMetadata_.set(controls::Brightness,\n> > > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::CONTRAST: {\n> > > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"contrast\"));\n> > > > - ASSERT(contrast);\n> > > > + if (!contrast) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set CONTRAST - no contrast algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > contrast->SetContrast(ctrl.second.get());\n> > > > libcameraMetadata_.set(controls::Contrast,\n> > > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::SATURATION: {\n> > > > RPiController::CcmAlgorithm *ccm = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"ccm\"));\n> > > > - ASSERT(ccm);\n> > > > + if (!ccm) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set SATURATION - no ccm algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > ccm->SetSaturation(ctrl.second.get());\n> > > > libcameraMetadata_.set(controls::Saturation,\n> > > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > case controls::SHARPNESS: {\n> > > > RPiController::SharpenAlgorithm *sharpen = dynamic_cast(\n> > > > controller_.GetAlgorithm(\"sharpen\"));\n> > > > - ASSERT(sharpen);\n> > > > + if (!sharpen) {\n> > > > + LOG(IPARPI, Warning)\n> > > > + << \"Could not set SHARPNESS - no sharpen algorithm\";\n> > > > + break;\n> > > > + }\n> > > >\n> > > > sharpen->SetStrength(ctrl.second.get());\n> > > > libcameraMetadata_.set(controls::Sharpness,\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 23BD6BD808\n\tfor ;\n\tWed, 27 Jan 2021 11:43:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A628760F2F;\n\tWed, 27 Jan 2021 12:43:48 +0100 (CET)","from mail-oi1-x231.google.com (mail-oi1-x231.google.com\n\t[IPv6:2607:f8b0:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66BA060F2F\n\tfor ;\n\tWed, 27 Jan 2021 12:43:46 +0100 (CET)","by mail-oi1-x231.google.com with SMTP id g69so1781860oib.12\n\tfor ;\n\tWed, 27 Jan 2021 03:43:46 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"q2trzffp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=WEeOlW4y35EI4baiQFdFRQsT2QA9tc0ktMIzCfsXd7Q=;\n\tb=q2trzffpP5Hw9so1372PLyheH3kDuzodmXcEX12H1gNaAeL8yZR5UxfOuKZu4tcDbR\n\tQBeyne7HWw6RIPvSRDmD78naCVOxafq9l+gFO57xRhO6l2/DOMqrV2Bey5hHqJoHNGqi\n\tvqLKoEI2dWJYreKoZQRlbWjZu5huEtWoNBqn04X5xbEC0ECNFcNBV3ToQgH847CuL6tX\n\tA1A69lypa8FXvujcUyJjovqlSeJDrA0YbOYUJZTQ0NMpW128WY9Hjt+4vhnd6X9jAGDU\n\t+vMuDlVljZtQcTMWfkSN/r+y/5gg8cMxTkfVoV2uTC2OD9gK9VnhKoTvfbjtt6nb7onm\n\tEdHw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=WEeOlW4y35EI4baiQFdFRQsT2QA9tc0ktMIzCfsXd7Q=;\n\tb=rPFqUSS891vQ+vJzACKNSDx4kcVHWlROxeaBcGC5RZl8OCmGcl9yoL4YE4S4/Uorfk\n\tlLu9KzPuLp3xFWaaX4UgMLeTuBwYBwgMDiZqbmdXxfDCYDNX0PDuQOlOrMU2IUBU8dCY\n\tse8LLMJ08AiyLh8RimhRHglkyMQpmbPtIEpztM39KZDdr1RO6RwkzG69/QpLHdCRzW00\n\t5vdJU8lzimU/oP9lRuVZEvPayQrUBYuYeUuxPl1ikpImYFlzOirS6jN3AuEQ2H+apbH7\n\tjDTBvLJd52j9xLGzFHApEIQreJ8uU0F5ELChkj0l7FUGprT3nwIZb0t+QV5xNBTyFOKo\n\tNuMw==","X-Gm-Message-State":"AOAM532hUDeWUZ4jjBVxVYE/S2Hf3hoKFn0MpHW6XhmxQnYNqGxGoqbf\n\t1fxeNCnpLBllSHq1dTYsYRH28JKvcshBnjcfAZvHFmj5Q1D1nA==","X-Google-Smtp-Source":"ABdhPJzUWH8/fjb8HMwgcbSz/YcwFORGg8HWoPDZ8UIlxnnEH/QXVnpcPetUoqQ9QwrJ4R/klsCGzvVM2mBfo3KVy6o=","X-Received":"by 2002:aca:5c0a:: with SMTP id q10mr2948765oib.55.1611747825135;\n\tWed, 27 Jan 2021 03:43:45 -0800 (PST)","MIME-Version":"1.0","References":"<20210126154724.7155-1-david.plowman@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 27 Jan 2021 11:43:34 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14834,"web_url":"https://patchwork.libcamera.org/comment/14834/","msgid":"","date":"2021-01-27T12:02:45","subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","submitter":{"id":27,"url":"https://patchwork.libcamera.org/api/people/27/","name":"Dave Stevenson","email":"dave.stevenson@raspberrypi.com"},"content":"On Wed, 27 Jan 2021 at 11:33, David Plowman\n wrote:\n>\n> Hi Dave\n>\n> Yes, I'm a bit in several minds over monochrome sensors. Should\n> algorithms know whether sensors are colour or monochrome? Should\n> certain algorithms just be deleted from the tuning file? Probably, but\n> it does want some thinking about.\n>\n> As regards this patch, I don't think anything gets worse (actually you\n> can delete the AWB and CCM algorithms and your camera will still run).\n> There should be just one grumble at startup when it tries to set the\n> AWB mode. AGC is actually more annoying, though it's not affected\n> either way by this patch, as it complains repeatedly if it has no AWB\n> metadata. Either the algorithm would have to know not to care, or\n> perhaps that warning isn't really very useful these days and could be\n> demoted.\n>\n> Certainly someone (e.g. me) needs to spend some time with these\n> monochrome sensors, calibrating them and making sure everything works.\n> The tuning tool will have issues with them too, but it would be great\n> to have more supported sensors. Do they all have fully functional V4L2\n> drivers for libcamera?\n\nFunctional V4L2 sensor drivers and dtoverlays - yes.\nThey'll need cam_helper implementations in order to work with libcamera.\n\nOV2311 isn't merged to rpi-5.10.y as yet as I only found a published\nregister set about a week ago.\nhttps://github.com/6by9/linux/tree/ov2311 is streaming images and has\nthe normal controls (calibration needs checking).\n\nOV7251 (0.3MP global shutter) and OV9281 (1MPix global shutter) are\nboth in rpi-5.10.y. I believe they all have the required V4L2 controls\nfor libcamera, but haven't double checked.\n\n Dave\n\n> Thanks!\n> David\n>\n> On Wed, 27 Jan 2021 at 11:00, Dave Stevenson\n> wrote:\n> >\n> > Hi David\n> >\n> > On Wed, 27 Jan 2021 at 10:44, David Plowman\n> > wrote:\n> > >\n> > > Hi Laurent\n> > >\n> > > Thanks for the comments. Let me just add a few more words for the\n> > > situation I'm thinking of.\n> > >\n> > > On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart\n> > > wrote:\n> > > >\n> > > > Hi David,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:\n> > > > > Users are free to avoid loading certain control algorithms from the\n> > > > > json tuning file if they wish. Under such circumstances, failing\n> > > > > completely when an application tries to set parameters for that\n> > > > > algorithm is unhelpful, and it is better just to issue a warning.\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----\n> > > > > 1 file changed, 72 insertions(+), 14 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > index 5ccc7a65..b57d77e9 100644\n> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > switch (ctrl.first) {\n> > > > > case controls::AE_ENABLE: {\n> > > > > RPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AE_ENABLE - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > >\n> > > > This is a better behaviour indeed. I wonder if we need some kind of\n> > > > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so\n> > > > per-camera.\n> > > >\n> > > > Not aborting when a control is set without a corresponding algorithm is\n> > > > good, but I think we need to go one step further and not report the\n> > > > control to the application in the first place. This will require\n> > > > replacing the static ControlInfoMap in\n> > > > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We\n> > > > should then extend the libcamera core to verify that all controls in a\n> > > > request are supported and either reject the request or strip the\n> > > > unsupported controls, and we won't need this patch anymore.\n> > > >\n> > > > I'm fine merging this patch as-is, given that the ongoing IPA IPC work\n> > > > should make it easier to handle controls dynamically when it will land,\n> > > > but it should then probably be reverted.\n> > >\n> > > So the specific thing that led to the change was the following.\n> > >\n> > > Normally I can run a libcamera application of my choice, such as qcam\n> > > or the Raspberry Pi libcamera-hello. Now, I might decide that I don't\n> > > want the control algorithms to touch (for example) the gamma curve any\n> > > more. So I comment the \"rpi.contrast\" algorithm out of the json file.\n> > >\n> > > Now, qcam will still run quite happily (I think it sets almost no\n> > > controls) but libcamera-hello (prior to this change) would assert and\n> > > fail. The reason is that it tries to set the brightness to the value\n> > > chosen by the user (or its default), and if there's no contrast\n> > > algorithm, it can't do this. So the idea is that omitting certain\n> > > algorithm(s) doesn't mean you have to start editing your application\n> > > code.\n> > >\n> > > However, I actually quite like the fact that the warning nags quite a\n> > > lot. It may well be that a single warning would go by during start-up\n> > > and is relatively easy to miss... how about a LOG_N_TIMES version?\n> >\n> > Unless I've misunderstood, won't this also apply for monochrome\n> > sensors where you won't want AWB to run as there is no colour? Logging\n> > any form of error or warning there is really unwanted.\n> >\n> > I have an increasing pile of mono sensors with working drivers that I\n> > can pass on if you wanted to try it out! (OV9281, OV7251, and OV2311\n> > for starters).\n> >\n> > Dave\n> >\n> > > Thanks!\n> > > David\n> > >\n> > > >\n> > > > > +\n> > > > > if (ctrl.second.get() == false)\n> > > > > agc->Pause();\n> > > > > else\n> > > > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::EXPOSURE_TIME: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set EXPOSURE_TIME - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > /* This expects units of micro-seconds. */\n> > > > > agc->SetFixedShutter(ctrl.second.get());\n> > > > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::ANALOGUE_GAIN: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set ANALOGUE_GAIN - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > > +\n> > > > > agc->SetFixedAnalogueGain(ctrl.second.get());\n> > > > >\n> > > > > libcameraMetadata_.set(controls::AnalogueGain,\n> > > > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::AE_METERING_MODE: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AE_METERING_MODE - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > int32_t idx = ctrl.second.get();\n> > > > > if (MeteringModeTable.count(idx)) {\n> > > > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::AE_CONSTRAINT_MODE: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AE_CONSTRAINT_MODE - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > int32_t idx = ctrl.second.get();\n> > > > > if (ConstraintModeTable.count(idx)) {\n> > > > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::AE_EXPOSURE_MODE: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AE_EXPOSURE_MODE - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > int32_t idx = ctrl.second.get();\n> > > > > if (ExposureModeTable.count(idx)) {\n> > > > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::EXPOSURE_VALUE: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set EXPOSURE_VALUE - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > /*\n> > > > > * The SetEv() method takes in a direct exposure multiplier.\n> > > > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > >\n> > > > > case controls::AWB_ENABLE: {\n> > > > > RPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> > > > > - ASSERT(awb);\n> > > > > + if (!awb) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AWB_ENABLE - no AWB algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > if (ctrl.second.get() == false)\n> > > > > awb->Pause();\n> > > > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::AWB_MODE: {\n> > > > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"awb\"));\n> > > > > - ASSERT(awb);\n> > > > > + if (!awb) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AWB_MODE - no AWB algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > int32_t idx = ctrl.second.get();\n> > > > > if (AwbModeTable.count(idx)) {\n> > > > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > auto gains = ctrl.second.get>();\n> > > > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"awb\"));\n> > > > > - ASSERT(awb);\n> > > > > + if (!awb) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set COLOUR_GAINS - no AWB algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > awb->SetManualGains(gains[0], gains[1]);\n> > > > > if (gains[0] != 0.0f && gains[1] != 0.0f)\n> > > > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::BRIGHTNESS: {\n> > > > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"contrast\"));\n> > > > > - ASSERT(contrast);\n> > > > > + if (!contrast) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set BRIGHTNESS - no contrast algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > contrast->SetBrightness(ctrl.second.get() * 65536);\n> > > > > libcameraMetadata_.set(controls::Brightness,\n> > > > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::CONTRAST: {\n> > > > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"contrast\"));\n> > > > > - ASSERT(contrast);\n> > > > > + if (!contrast) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set CONTRAST - no contrast algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > contrast->SetContrast(ctrl.second.get());\n> > > > > libcameraMetadata_.set(controls::Contrast,\n> > > > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::SATURATION: {\n> > > > > RPiController::CcmAlgorithm *ccm = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"ccm\"));\n> > > > > - ASSERT(ccm);\n> > > > > + if (!ccm) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set SATURATION - no ccm algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > ccm->SetSaturation(ctrl.second.get());\n> > > > > libcameraMetadata_.set(controls::Saturation,\n> > > > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::SHARPNESS: {\n> > > > > RPiController::SharpenAlgorithm *sharpen = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"sharpen\"));\n> > > > > - ASSERT(sharpen);\n> > > > > + if (!sharpen) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set SHARPNESS - no sharpen algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > sharpen->SetStrength(ctrl.second.get());\n> > > > > libcameraMetadata_.set(controls::Sharpness,\n> > > >\n> > > > --\n> > > > Regards,\n> > > >\n> > > > Laurent Pinchart\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel","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 67B7EBD808\n\tfor ;\n\tWed, 27 Jan 2021 12:03:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E7C676836C;\n\tWed, 27 Jan 2021 13:03:01 +0100 (CET)","from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com\n\t[IPv6:2a00:1450:4864:20::42f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 96C766836C\n\tfor ;\n\tWed, 27 Jan 2021 13:03:00 +0100 (CET)","by mail-wr1-x42f.google.com with SMTP id z6so1575814wrq.10\n\tfor ;\n\tWed, 27 Jan 2021 04:03:00 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"mA8tqUVz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=wiJWpq5Ckh28l6Ld6fyMCqn8650QkECc5SsvTEsi/BY=;\n\tb=mA8tqUVzZ+Iizy8Ma6910yLb3uGEpN4ZLwvLRY/BzI+6FFflplkII8pbBosMpkQg5K\n\tWfrfKvDNM2R+Vc9VHa6AWeq58qAREdHzKgFVBIJ3ew5XivqgHhU12segejxz4o2JwYxm\n\tqVSlHxnvioWwPEvHCZFtv6Tdm6T649JL3dWhyUZ2lz2yAOxUUCJUU7zIMdrYKiTcbJKR\n\tlgBJj56956IDD2bsZGfg+AvDV46qUdX5V/JOvi8wPapliOjKR+JcNeCM5U6K6piJcfoj\n\tusjsrw3hNOY7jKvv7gGmgTtuXTUidLda+YJ2RKCxEJmLTfLIeQLOKXwXXFJTwDAlrzVl\n\tb7DA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=wiJWpq5Ckh28l6Ld6fyMCqn8650QkECc5SsvTEsi/BY=;\n\tb=oRQKJfu5b2/9ZRDJNEIJqIVKzZKSlr0XI9sMMcW6P148h+4KrIToFcfllGhjn46EEU\n\tToVpq6xjjOyqHZ9lgMo6yAA6oENNUkuSKO36BKn08CWNqwqPBjuT4dXGuUhMVkdpx0uP\n\tWLmNDsIdgbvdRi0lk70KUOL4sw2Wgv2mFvZ4kuO5Q8d1KgOgB9wVtJbar/JKt8H/BVhf\n\t9y66wjNZzfdKvwo3PYBes/ajXN/hqdW6if2InW3hlMDo7wFZYzgEWTnpNYbs3bO3tJa3\n\txrq1LV9HA172rcomL3D2ETo50Y5wyNLS7XyYG38kxBCkUjFsT8KpzAcRiWmkoGks67z+\n\t3NWg==","X-Gm-Message-State":"AOAM533U76rpnpOdpA8NUk4z5eRL9tf0HuZXtlQ2tKNbFW0CD0k4ut8j\n\t9MLnfAP6OYSqESbJ2ygRL6ql16vey8MGRktWrsgseg==","X-Google-Smtp-Source":"ABdhPJxNw+mYSg9gM/9qeHmjj6sz58CbH6ONu9jsz99M/2jbYzPbPSG4tWr8hGyAtj18x/nu2AxgIn/p54rJ+vK4l5k=","X-Received":"by 2002:a5d:4882:: with SMTP id\n\tg2mr10678713wrq.273.1611748980156; \n\tWed, 27 Jan 2021 04:03:00 -0800 (PST)","MIME-Version":"1.0","References":"<20210126154724.7155-1-david.plowman@raspberrypi.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Dave Stevenson ","Date":"Wed, 27 Jan 2021 12:02:45 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14882,"web_url":"https://patchwork.libcamera.org/comment/14882/","msgid":"","date":"2021-02-01T15:27:34","subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nI was wondering if I might give this one another little nudge. I've\nbeen working with a different sensor for a few days using the\n\"uncalibrated.json\" tuning... only the Raspberry Pi libcamera-apps all\nfail without the patch because that json file only loads a skeleton\nset of control algorithms.\n\nI'm fine to change this back once we have dynamic controls - perhaps\nthat wants a \"\\todo\" in the commit message or the source file? I'd be\nhappy to add that and re-submit.\n\nThanks!\nDavid\n\nOn Wed, 27 Jan 2021 at 11:43, David Plowman\n wrote:\n>\n> Hi Laurent\n>\n> On Wed, 27 Jan 2021 at 11:17, Laurent Pinchart\n> wrote:\n> >\n> > Hi David,\n> >\n> > On Wed, Jan 27, 2021 at 10:44:19AM +0000, David Plowman wrote:\n> > > Hi Laurent\n> > >\n> > > Thanks for the comments. Let me just add a few more words for the\n> > > situation I'm thinking of.\n> > >\n> > > On Wed, 27 Jan 2021 at 10:19, Laurent Pinchart wrote:\n> > > > On Tue, Jan 26, 2021 at 03:47:24PM +0000, David Plowman wrote:\n> > > > > Users are free to avoid loading certain control algorithms from the\n> > > > > json tuning file if they wish. Under such circumstances, failing\n> > > > > completely when an application tries to set parameters for that\n> > > > > algorithm is unhelpful, and it is better just to issue a warning.\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > src/ipa/raspberrypi/raspberrypi.cpp | 86 ++++++++++++++++++++++++-----\n> > > > > 1 file changed, 72 insertions(+), 14 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > index 5ccc7a65..b57d77e9 100644\n> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > @@ -627,7 +627,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > switch (ctrl.first) {\n> > > > > case controls::AE_ENABLE: {\n> > > > > RPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AE_ENABLE - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > >\n> > > > This is a better behaviour indeed. I wonder if we need some kind of\n> > > > LOG_ONCE() to avoid flooding the log, but it would be difficult to do so\n> > > > per-camera.\n> > > >\n> > > > Not aborting when a control is set without a corresponding algorithm is\n> > > > good, but I think we need to go one step further and not report the\n> > > > control to the application in the first place. This will require\n> > > > replacing the static ControlInfoMap in\n> > > > include/libcamera/ipa/raspberrypi.h with a more dynamic mechanism. We\n> > > > should then extend the libcamera core to verify that all controls in a\n> > > > request are supported and either reject the request or strip the\n> > > > unsupported controls, and we won't need this patch anymore.\n> > > >\n> > > > I'm fine merging this patch as-is, given that the ongoing IPA IPC work\n> > > > should make it easier to handle controls dynamically when it will land,\n> > > > but it should then probably be reverted.\n> > >\n> > > So the specific thing that led to the change was the following.\n> > >\n> > > Normally I can run a libcamera application of my choice, such as qcam\n> > > or the Raspberry Pi libcamera-hello. Now, I might decide that I don't\n> > > want the control algorithms to touch (for example) the gamma curve any\n> > > more. So I comment the \"rpi.contrast\" algorithm out of the json file.\n> > >\n> > > Now, qcam will still run quite happily (I think it sets almost no\n> > > controls) but libcamera-hello (prior to this change) would assert and\n> > > fail. The reason is that it tries to set the brightness to the value\n> > > chosen by the user (or its default), and if there's no contrast\n> > > algorithm, it can't do this. So the idea is that omitting certain\n> > > algorithm(s) doesn't mean you have to start editing your application\n> > > code.\n> >\n> > The API concept is that applications should only set controls that are\n> > reported by the camera as supported. That puts a bit of an extra burden\n> > on the application, but for applications that don't care about controls\n> > being ignored if unsupported, it would be easy to implement a small\n> > control set wrapper function that would skip controls that are not\n> > advertised.\n>\n> Yes, I understand you now. We need \"dynamic controls\" which will\n> depend in our case probably on the control algorithms that have been\n> loaded. Checking whether controls are actually supported is perhaps a\n> little tiresome, but I don't particularly object to our applications\n> doing that.\n>\n> Thanks for the clarifications!\n> Davd\n>\n> >\n> > > However, I actually quite like the fact that the warning nags quite a\n> > > lot. It may well be that a single warning would go by during start-up\n> > > and is relatively easy to miss... how about a LOG_N_TIMES version?\n> > >\n> > > > > +\n> > > > > if (ctrl.second.get() == false)\n> > > > > agc->Pause();\n> > > > > else\n> > > > > @@ -640,7 +645,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::EXPOSURE_TIME: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set EXPOSURE_TIME - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > /* This expects units of micro-seconds. */\n> > > > > agc->SetFixedShutter(ctrl.second.get());\n> > > > > @@ -652,7 +661,12 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::ANALOGUE_GAIN: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set ANALOGUE_GAIN - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > > +\n> > > > > agc->SetFixedAnalogueGain(ctrl.second.get());\n> > > > >\n> > > > > libcameraMetadata_.set(controls::AnalogueGain,\n> > > > > @@ -663,7 +677,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::AE_METERING_MODE: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AE_METERING_MODE - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > int32_t idx = ctrl.second.get();\n> > > > > if (MeteringModeTable.count(idx)) {\n> > > > > @@ -679,7 +697,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::AE_CONSTRAINT_MODE: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AE_CONSTRAINT_MODE - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > int32_t idx = ctrl.second.get();\n> > > > > if (ConstraintModeTable.count(idx)) {\n> > > > > @@ -695,7 +717,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::AE_EXPOSURE_MODE: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AE_EXPOSURE_MODE - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > int32_t idx = ctrl.second.get();\n> > > > > if (ExposureModeTable.count(idx)) {\n> > > > > @@ -711,7 +737,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::EXPOSURE_VALUE: {\n> > > > > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"agc\"));\n> > > > > - ASSERT(agc);\n> > > > > + if (!agc) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set EXPOSURE_VALUE - no AGC algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > /*\n> > > > > * The SetEv() method takes in a direct exposure multiplier.\n> > > > > @@ -726,7 +756,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > >\n> > > > > case controls::AWB_ENABLE: {\n> > > > > RPiController::Algorithm *awb = controller_.GetAlgorithm(\"awb\");\n> > > > > - ASSERT(awb);\n> > > > > + if (!awb) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AWB_ENABLE - no AWB algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > if (ctrl.second.get() == false)\n> > > > > awb->Pause();\n> > > > > @@ -741,7 +775,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::AWB_MODE: {\n> > > > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"awb\"));\n> > > > > - ASSERT(awb);\n> > > > > + if (!awb) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set AWB_MODE - no AWB algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > int32_t idx = ctrl.second.get();\n> > > > > if (AwbModeTable.count(idx)) {\n> > > > > @@ -758,7 +796,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > auto gains = ctrl.second.get>();\n> > > > > RPiController::AwbAlgorithm *awb = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"awb\"));\n> > > > > - ASSERT(awb);\n> > > > > + if (!awb) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set COLOUR_GAINS - no AWB algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > awb->SetManualGains(gains[0], gains[1]);\n> > > > > if (gains[0] != 0.0f && gains[1] != 0.0f)\n> > > > > @@ -771,7 +813,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::BRIGHTNESS: {\n> > > > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"contrast\"));\n> > > > > - ASSERT(contrast);\n> > > > > + if (!contrast) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set BRIGHTNESS - no contrast algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > contrast->SetBrightness(ctrl.second.get() * 65536);\n> > > > > libcameraMetadata_.set(controls::Brightness,\n> > > > > @@ -782,7 +828,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::CONTRAST: {\n> > > > > RPiController::ContrastAlgorithm *contrast = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"contrast\"));\n> > > > > - ASSERT(contrast);\n> > > > > + if (!contrast) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set CONTRAST - no contrast algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > contrast->SetContrast(ctrl.second.get());\n> > > > > libcameraMetadata_.set(controls::Contrast,\n> > > > > @@ -793,7 +843,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::SATURATION: {\n> > > > > RPiController::CcmAlgorithm *ccm = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"ccm\"));\n> > > > > - ASSERT(ccm);\n> > > > > + if (!ccm) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set SATURATION - no ccm algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > ccm->SetSaturation(ctrl.second.get());\n> > > > > libcameraMetadata_.set(controls::Saturation,\n> > > > > @@ -804,7 +858,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > > case controls::SHARPNESS: {\n> > > > > RPiController::SharpenAlgorithm *sharpen = dynamic_cast(\n> > > > > controller_.GetAlgorithm(\"sharpen\"));\n> > > > > - ASSERT(sharpen);\n> > > > > + if (!sharpen) {\n> > > > > + LOG(IPARPI, Warning)\n> > > > > + << \"Could not set SHARPNESS - no sharpen algorithm\";\n> > > > > + break;\n> > > > > + }\n> > > > >\n> > > > > sharpen->SetStrength(ctrl.second.get());\n> > > > > libcameraMetadata_.set(controls::Sharpness,\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 83DB6C33BB\n\tfor ;\n\tMon, 1 Feb 2021 15:27:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DFBB6840C;\n\tMon, 1 Feb 2021 16:27:49 +0100 (CET)","from mail-ot1-x330.google.com (mail-ot1-x330.google.com\n\t[IPv6:2607:f8b0:4864:20::330])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D8F0683FF\n\tfor ;\n\tMon, 1 Feb 2021 16:27:47 +0100 (CET)","by mail-ot1-x330.google.com with SMTP id k8so16623172otr.8\n\tfor ;\n\tMon, 01 Feb 2021 07:27:47 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"jHyVJrZh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=SLO+dfknxz4TTdMdWJ5u/6WFC+nKvCbSFpKatKplRUY=;\n\tb=jHyVJrZhscTb99PqlqFZdIQGtGbObboigvgDruylrGRy0FlroKctCSo3Xfr3B0TnbG\n\t+UD568WTq8/NfI6o4AxGkSAPzsgvE+xpvlUAUChWTjwHBxLCfjdIVlvnOL0nQU0QXJsv\n\tFsxx91mQ52JHdezkJ66NmzO6dicwarQhIE98pUT4Zk3rtf1+0Elm5RgkcJ57r0brcJdT\n\t4Ar57vNQGFnD+sccvV0XAMrJdQ+XkgDkGb5kmHFuUabfusuQYWeXKGkw8fFnDyzSdw4t\n\tt3KfRimbTK4X+bL3LuR0Tb3932sOKDgyZqicET9oe9xAn7JsIwqhig3ek4IQRmToC+nR\n\t5Dpg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=SLO+dfknxz4TTdMdWJ5u/6WFC+nKvCbSFpKatKplRUY=;\n\tb=hJ7jos87WJfb8EZzaU0TiY/KTDDUFzcXK/HiD3UC58SOrquWrumd7JoBLJap7xL5fv\n\tuOJttYNJizPph2c0i52COC4xLWx/a4GhsX7ucADs3/s40aYuXFpiLXFkw1RWhe45T8zi\n\tFkw/Qx0h8XKrbK2ubyLVq5fLBiKG5N9irJoI9L0/JRLMGWCiAjf2XCX2QLvChq6x22Ii\n\tuOYPrQopk735Z8z/nAlzT6G0hrGQ0fglIeV4UBHyNQlFdW87ufW2pkzkWYRSdqkEiJz+\n\tDUGq7ufdexSp+FFiCZzX7YrJyL9q/JdeIsq0Oo417s3wB5OEMtA5eWCV14/09iwCS2OB\n\t3HlQ==","X-Gm-Message-State":"AOAM5332QGnPL7Uby/xmvSSpfrNJo65gN/WixmnCqJlcKDyywk+hB/pa\n\tfnBm0Nh08hmeNzTLsky/Up1FLt9zjvTCkRsdhgiJJ0Sb3b3gCQ==","X-Google-Smtp-Source":"ABdhPJztIWtTInm7gNnFj8VEOHu6b40wB3spPEyF0kDk0FbBv6wpJyUgLlOandFfmElLvTsAw/yt7SDYGYlX1CpxdgQ=","X-Received":"by 2002:a9d:6852:: with SMTP id\n\tc18mr12438340oto.166.1612193265645; \n\tMon, 01 Feb 2021 07:27:45 -0800 (PST)","MIME-Version":"1.0","References":"<20210126154724.7155-1-david.plowman@raspberrypi.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Mon, 1 Feb 2021 15:27:34 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] ipa: raspberrypi: Warn when control\n\talgorithms are missing; do not fail","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]