[{"id":16620,"web_url":"https://patchwork.libcamera.org/comment/16620/","msgid":"","date":"2021-04-27T05:18:10","subject":"Re: [libcamera-devel] [RFC PATCH v2 11/12] pipeline: ipu3: Set\n\trequest metadata for FULL compliance","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Apr 22, 2021 at 06:41:01PM +0900, Paul Elder wrote:\n> Set the request metadata as required by FULL hardware level.\n\nLet's document that this isn't the final implementation, controls need\nto be plumbed to the IPA.\n\n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 67 ++++++++++++++++++++++++++--\n> 1 file changed, 64 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 70a5e9ce..de90b9fe 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -75,7 +75,8 @@ class IPU3CameraData : public CameraData\n> {\n> public:\n> \tIPU3CameraData(PipelineHandler *pipe)\n> -\t\t: CameraData(pipe), exposureTime_(0), supportsFlips_(false)\n> +\t\t: CameraData(pipe), exposureTime_(0), supportsFlips_(false),\n> +\t\t lastTimestamp_(0)\n> \t{\n> \t}\n> \n> @@ -106,6 +107,8 @@ public:\n> private:\n> \tvoid queueFrameAction(unsigned int id,\n> \t\t\t const ipa::ipu3::IPU3Action &action);\n> +\n> +\tint64_t lastTimestamp_;\n> };\n> \n> class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -1249,12 +1252,65 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> \n> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> \t/* \\todo Move the ExposureTime control to the IPA. */\n> -\trequest->metadata().set(controls::ExposureTime, exposureTime_);\n> +\trequest->metadata().set(controls::ExposureTime,\n> +\t\t\t\trequest->controls().contains(controls::ExposureTime) ?\n> +\t\t\t\trequest->controls().get(controls::ExposureTime) :\n> +\t\t\t\texposureTime_);\n> \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> \tif (request->controls().contains(controls::ScalerCrop))\n> \t\tcropRegion_ = request->controls().get(controls::ScalerCrop);\n> \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n> \n> +\trequest->metadata().set(controls::draft::BlackLevelLocked,\n> +\t\t\t\trequest->controls().contains(controls::draft::BlackLevelLocked) ?\n> +\t\t\t request->controls().get(controls::draft::BlackLevelLocked) :\n> +\t\t\t\tfalse);\n\nLots of copy & paste boilerplate. It should ideally be replaced with a\n(template) helper function. As this will go away anyway I'm OK with it\nfor the time being.\n\n> +\n> +\trequest->metadata().set(controls::AeLocked,\n> +\t\t\t\trequest->controls().contains(controls::AeLocked) ?\n> +\t\t\t\trequest->controls().get(controls::AeLocked) :\n> +\t\t\t\tfalse);\n> +\n> +\trequest->metadata().set(controls::draft::AePrecaptureTrigger,\n> +\t\t\t\trequest->controls().contains(controls::draft::AePrecaptureTrigger) ?\n> +\t\t\t\trequest->controls().get(controls::draft::AePrecaptureTrigger) :\n> +\t\t\t\tcontrols::draft::AePrecaptureTriggerIdle);\n> +\n> +\trequest->metadata().set(controls::AwbMode,\n> +\t\t\t\trequest->controls().contains(controls::AwbMode) ?\n> +\t\t\t\trequest->controls().get(controls::AwbMode) :\n> +\t\t\t\tcontrols::AwbAuto);\n> +\n> +\trequest->metadata().set(controls::AwbLocked,\n> +\t\t\t\trequest->controls().contains(controls::AwbLocked) ?\n> +\t\t\t\trequest->controls().get(controls::AwbLocked) :\n> +\t\t\t\tfalse);\n> +\n> +\trequest->metadata().set(controls::draft::EdgeMode,\n> +\t\t\t\trequest->controls().contains(controls::draft::EdgeMode) ?\n> +\t\t\t\trequest->controls().get(controls::draft::EdgeMode) :\n> +\t\t\t\t(uint8_t)controls::draft::EdgeModeOff);\n> +\n> +\trequest->metadata().set(controls::draft::NoiseReductionMode,\n> +\t\t\t\trequest->controls().contains(controls::draft::NoiseReductionMode) ?\n> +\t\t\t\trequest->controls().get(controls::draft::NoiseReductionMode) :\n> +\t\t\t\tcontrols::draft::NoiseReductionModeOff);\n> +\n> +\trequest->metadata().set(controls::draft::SensorSensitivity,\n> +\t\t\t\trequest->controls().contains(controls::draft::SensorSensitivity) ?\n> +\t\t\t\trequest->controls().get(controls::draft::SensorSensitivity) :\n> +\t\t\t\t32);\n> +\n> +\tif (request->metadata().get(controls::draft::FrameDuration) <\n> +\t request->metadata().get(controls::ExposureTime) * 1000)\n> +\t\t\trequest->metadata().set(controls::draft::FrameDuration,\n> +\t\t\t\t\t\trequest->metadata().get(controls::ExposureTime) * 1000);\n> +\n> +\trequest->metadata().set(controls::draft::TonemapMode,\n> +\t\t\t\trequest->controls().contains(controls::draft::TonemapMode) ?\n> +\t\t\t\trequest->controls().get(controls::draft::TonemapMode) :\n> +\t\t\t\t(uint8_t)controls::draft::TonemapModeFast);\n> +\n> \tif (frameInfos_.tryComplete(info))\n> \t\tpipe_->completeRequest(request);\n> }\n> @@ -1280,8 +1336,13 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \t * \\todo The sensor timestamp should be better estimated by connecting\n> \t * to the V4L2Device::frameStart signal.\n> \t */\n> +\tint64_t timestamp = buffer->metadata().timestamp;\n> \trequest->metadata().set(controls::SensorTimestamp,\n> -\t\t\t\tbuffer->metadata().timestamp);\n> +\t\t\t\ttimestamp);\n> +\n> +\trequest->metadata().set(controls::draft::FrameDuration,\n> +\t\t\t\ttimestamp - lastTimestamp_);\n> +\tlastTimestamp_ = timestamp;\n\nSame comment as before, this should not be measured.\n\n> \n> \t/* If the buffer is cancelled force a complete of the whole request. */\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {","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 CED8ABDCA6\n\tfor ;\n\tTue, 27 Apr 2021 05:18:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5581268878;\n\tTue, 27 Apr 2021 07:18:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49DCF60512\n\tfor ;\n\tTue, 27 Apr 2021 07:18:16 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A787FE9;\n\tTue, 27 Apr 2021 07:18:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jImqrrHJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619500695;\n\tbh=x0Dpztox/KGHPwmZ6jxX93HjKQJqNFhXUDw9RjNDQFs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jImqrrHJe81/lFAMXbmc4SHih+m9ozWgx6PC8zIvdCT5v1EOD15D8M5ANQ4BCyWNc\n\tId3o34Y+DvY/13opcQOoYz9J0tQp+HgIkwA7hR/+DiQLhTmSpGEW9ZMF/OXlUzApoH\n\tbLQa9tRRBkWcmeCpYqMr2IlXpLEPMD73iXfzuFHE=","Date":"Tue, 27 Apr 2021 08:18:10 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210422094102.371772-1-paul.elder@ideasonboard.com>\n\t<20210422094102.371772-12-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422094102.371772-12-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 11/12] pipeline: ipu3: Set\n\trequest metadata for FULL compliance","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":16657,"web_url":"https://patchwork.libcamera.org/comment/16657/","msgid":"<20210427105251.6dygp5integc5kuy@uno.localdomain>","date":"2021-04-27T10:52:51","subject":"Re: [libcamera-devel] [RFC PATCH v2 11/12] pipeline: ipu3: Set\n\trequest metadata for FULL compliance","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Thu, Apr 22, 2021 at 06:41:01PM +0900, Paul Elder wrote:\n> Set the request metadata as required by FULL hardware level.\n>\n> Signed-off-by: Paul Elder \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 67 ++++++++++++++++++++++++++--\n> 1 file changed, 64 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 70a5e9ce..de90b9fe 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -75,7 +75,8 @@ class IPU3CameraData : public CameraData\n> {\n> public:\n> \tIPU3CameraData(PipelineHandler *pipe)\n> -\t\t: CameraData(pipe), exposureTime_(0), supportsFlips_(false)\n> +\t\t: CameraData(pipe), exposureTime_(0), supportsFlips_(false),\n> +\t\t lastTimestamp_(0)\n> \t{\n> \t}\n>\n> @@ -106,6 +107,8 @@ public:\n> private:\n> \tvoid queueFrameAction(unsigned int id,\n> \t\t\t const ipa::ipu3::IPU3Action &action);\n> +\n> +\tint64_t lastTimestamp_;\n> };\n>\n> class IPU3CameraConfiguration : public CameraConfiguration\n> @@ -1249,12 +1252,65 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n>\n> \trequest->metadata().set(controls::draft::PipelineDepth, 3);\n> \t/* \\todo Move the ExposureTime control to the IPA. */\n> -\trequest->metadata().set(controls::ExposureTime, exposureTime_);\n> +\trequest->metadata().set(controls::ExposureTime,\n> +\t\t\t\trequest->controls().contains(controls::ExposureTime) ?\n> +\t\t\t\trequest->controls().get(controls::ExposureTime) :\n> +\t\t\t\texposureTime_);\n> \t/* \\todo Actually apply the scaler crop region to the ImgU. */\n> \tif (request->controls().contains(controls::ScalerCrop))\n> \t\tcropRegion_ = request->controls().get(controls::ScalerCrop);\n> \trequest->metadata().set(controls::ScalerCrop, cropRegion_);\n>\n> +\trequest->metadata().set(controls::draft::BlackLevelLocked,\n> +\t\t\t\trequest->controls().contains(controls::draft::BlackLevelLocked) ?\n> +\t\t\t request->controls().get(controls::draft::BlackLevelLocked) :\n> +\t\t\t\tfalse);\n\nI feel like we're missing a policy here\nWhat is the meaning of:\n\nFrame Control Present? Value\n1 BlackLevelLocked yes true\n2 BlackLevelLocked no\n3 BlackLevelLocked yes false\n\nShould the black level lock be held in frame 2) or should be removed ?\nDo we want application to -always- specify all the control values, or\ndo we assume if a control is not there it has not changed value ?\nThis has impacts on the IPA implementation too, let alone the HAL\n(would be interesting how gstreamer behaves, for example)\n\nThanks\n j\n\n> +\n> +\trequest->metadata().set(controls::AeLocked,\n> +\t\t\t\trequest->controls().contains(controls::AeLocked) ?\n> +\t\t\t\trequest->controls().get(controls::AeLocked) :\n> +\t\t\t\tfalse);\n> +\n> +\trequest->metadata().set(controls::draft::AePrecaptureTrigger,\n> +\t\t\t\trequest->controls().contains(controls::draft::AePrecaptureTrigger) ?\n> +\t\t\t\trequest->controls().get(controls::draft::AePrecaptureTrigger) :\n> +\t\t\t\tcontrols::draft::AePrecaptureTriggerIdle);\n> +\n> +\trequest->metadata().set(controls::AwbMode,\n> +\t\t\t\trequest->controls().contains(controls::AwbMode) ?\n> +\t\t\t\trequest->controls().get(controls::AwbMode) :\n> +\t\t\t\tcontrols::AwbAuto);\n> +\n> +\trequest->metadata().set(controls::AwbLocked,\n> +\t\t\t\trequest->controls().contains(controls::AwbLocked) ?\n> +\t\t\t\trequest->controls().get(controls::AwbLocked) :\n> +\t\t\t\tfalse);\n> +\n> +\trequest->metadata().set(controls::draft::EdgeMode,\n> +\t\t\t\trequest->controls().contains(controls::draft::EdgeMode) ?\n> +\t\t\t\trequest->controls().get(controls::draft::EdgeMode) :\n> +\t\t\t\t(uint8_t)controls::draft::EdgeModeOff);\n> +\n> +\trequest->metadata().set(controls::draft::NoiseReductionMode,\n> +\t\t\t\trequest->controls().contains(controls::draft::NoiseReductionMode) ?\n> +\t\t\t\trequest->controls().get(controls::draft::NoiseReductionMode) :\n> +\t\t\t\tcontrols::draft::NoiseReductionModeOff);\n> +\n> +\trequest->metadata().set(controls::draft::SensorSensitivity,\n> +\t\t\t\trequest->controls().contains(controls::draft::SensorSensitivity) ?\n> +\t\t\t\trequest->controls().get(controls::draft::SensorSensitivity) :\n> +\t\t\t\t32);\n> +\n> +\tif (request->metadata().get(controls::draft::FrameDuration) <\n> +\t request->metadata().get(controls::ExposureTime) * 1000)\n> +\t\t\trequest->metadata().set(controls::draft::FrameDuration,\n> +\t\t\t\t\t\trequest->metadata().get(controls::ExposureTime) * 1000);\n> +\n> +\trequest->metadata().set(controls::draft::TonemapMode,\n> +\t\t\t\trequest->controls().contains(controls::draft::TonemapMode) ?\n> +\t\t\t\trequest->controls().get(controls::draft::TonemapMode) :\n> +\t\t\t\t(uint8_t)controls::draft::TonemapModeFast);\n> +\n> \tif (frameInfos_.tryComplete(info))\n> \t\tpipe_->completeRequest(request);\n> }\n> @@ -1280,8 +1336,13 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \t * \\todo The sensor timestamp should be better estimated by connecting\n> \t * to the V4L2Device::frameStart signal.\n> \t */\n> +\tint64_t timestamp = buffer->metadata().timestamp;\n> \trequest->metadata().set(controls::SensorTimestamp,\n> -\t\t\t\tbuffer->metadata().timestamp);\n> +\t\t\t\ttimestamp);\n> +\n> +\trequest->metadata().set(controls::draft::FrameDuration,\n> +\t\t\t\ttimestamp - lastTimestamp_);\n> +\tlastTimestamp_ = timestamp;\n>\n> \t/* If the buffer is cancelled force a complete of the whole request. */\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> --\n> 2.27.0\n>\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 D5DB8BDE19\n\tfor ;\n\tTue, 27 Apr 2021 10:52:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4EC4568866;\n\tTue, 27 Apr 2021 12:52:11 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 16D5F602C2\n\tfor ;\n\tTue, 27 Apr 2021 12:52:10 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 7F139E0005;\n\tTue, 27 Apr 2021 10:52:09 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Tue, 27 Apr 2021 12:52:51 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210427105251.6dygp5integc5kuy@uno.localdomain>","References":"<20210422094102.371772-1-paul.elder@ideasonboard.com>\n\t<20210422094102.371772-12-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422094102.371772-12-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 11/12] pipeline: ipu3: Set\n\trequest metadata for FULL compliance","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\" "}}]