[{"id":20833,"web_url":"https://patchwork.libcamera.org/comment/20833/","msgid":"<163658635101.2121661.15993065701698632915@Monstersaurus>","date":"2021-11-10T23:19:11","subject":"Re: [libcamera-devel] [PATCH v2 13/14] ipa: ipu3: Move ExposureTime\n\tto IPA","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-11-10 19:59:00)\n> Now that we have the exposure time calculated, pass it to the\n> controls::ExposureTime and don't use the pipeline handler for it\n> anymore. While at it, use the same line duration value for ExposureTime\n> and FrameDuration.\n> \n> Signed-off-by: Jean-Michel Hautbois \n> ---\n> src/ipa/ipu3/ipu3.cpp | 6 ++++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +---------------\n> 2 files changed, 5 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 68a97e8e..9220fea5 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -629,12 +629,14 @@ void IPAIPU3::parseStatistics(unsigned int frame,\n> setControls(frame);\n> \n> /* \\todo Use VBlank value calculated from each frame exposure. */\n> - int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /\n> - (sensorInfo_.pixelRate / 1e6);\n> + int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height)\n> + * lineDuration_.count();\n\nSame comments as previous patch regarding .count() vs .get(),\nbut it's optional. Though here it looks like it reads nicely to express\nthat it's in nanoseconds...\n\n> ctrls.set(controls::FrameDuration, frameDuration);\n> \n> ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK);\n> \n> + ctrls.set(controls::ExposureTime, context_.frameContext.agc.exposure * lineDuration_.count());\n> +\n\nI wonder if there should be any specific grouping or ordering in how we\nset the metadata controls here ...\n\nI guess it would end up being either alphabetical if sorted by control\nname, or grouped if things like exposure/gain should be kept together -\nor otherwise grouped by 'feature'?\n\n> /*\n> * \\todo We should be able to add 'anything' (with a Control) in here to\n> * get information to say.\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 3fcfa777..fe37e9c7 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -47,7 +47,7 @@ class IPU3CameraData : public Camera::Private\n> {\n> public:\n> IPU3CameraData(PipelineHandler *pipe)\n> - : Camera::Private(pipe), exposureTime_(0), supportsFlips_(false)\n> + : Camera::Private(pipe), supportsFlips_(false)\n> {\n> }\n> \n> @@ -67,7 +67,6 @@ public:\n> Stream vfStream_;\n> Stream rawStream_;\n> \n> - uint32_t exposureTime_;\n> Rectangle cropRegion_;\n> bool supportsFlips_;\n> Transform rotationTransform_;\n> @@ -1049,17 +1048,6 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)\n> \n> controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> \n> - /*\n> - * \\todo Report the actual exposure time, use the default for the\n> - * moment.\n> - */\n> - const auto exposureInfo = data->ipaControls_.find(&controls::ExposureTime);\n> - if (exposureInfo == data->ipaControls_.end()) {\n> - LOG(IPU3, Error) << \"Exposure control not initialized by the IPA\";\n> - return -EINVAL;\n> - }\n> - data->exposureTime_ = exposureInfo->second.def().get();\n> -\n> /* Add the IPA registered controls to list of camera controls. */\n> for (const auto &ipaControl : data->ipaControls_)\n> controls[ipaControl.first] = ipaControl.second;\n> @@ -1321,8 +1309,6 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)\n> pipe()->completeBuffer(request, buffer);\n> \n> request->metadata().set(controls::draft::PipelineDepth, 3);\n> - /* \\todo Move the ExposureTime control to the IPA. */\n> - request->metadata().set(controls::ExposureTime, exposureTime_);\n\nI'd sneak a blank separator line in here at this point ;-) But hey\nthat's me and my blank lines ....\n\nNothing non-trivial in the comments, so when considered how you see fit:\n\n\nReviewed-by: Kieran Bingham \n\n\n> /* \\todo Actually apply the scaler crop region to the ImgU. */\n> if (request->controls().contains(controls::ScalerCrop))\n> cropRegion_ = request->controls().get(controls::ScalerCrop);\n> -- \n> 2.32.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 56CE3BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 23:19:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6FC66034E;\n\tThu, 11 Nov 2021 00:19:14 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93C66600BF\n\tfor ;\n\tThu, 11 Nov 2021 00:19:13 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 31119292;\n\tThu, 11 Nov 2021 00:19:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eGFry3jX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636586353;\n\tbh=q6w/RLri4d186uGsvJunne+LZa3lB9iYL7N+Uwr8LwM=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=eGFry3jXHYaiwNBwXr86l8gpN7AeiYmjrsACQC1m7JuPAg4CJV5UdAAl2ScyT3P0K\n\tlcSmPiE1rdnDkCBh7Pwxqt1gKAByjyOkUd3hbvgeV/91ZLlqbR/qwvTJ/C9g1c2vDp\n\tQIvB1NHI6HXZo84PtGoGSzhzTs6TiAScfgqlJCuM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211110195901.85597-14-jeanmichel.hautbois@ideasonboard.com>","References":"<20211110195901.85597-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211110195901.85597-14-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 10 Nov 2021 23:19:11 +0000","Message-ID":"<163658635101.2121661.15993065701698632915@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 13/14] ipa: ipu3: Move ExposureTime\n\tto IPA","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]