[{"id":17280,"web_url":"https://patchwork.libcamera.org/comment/17280/","msgid":"","date":"2021-05-26T13:53:29","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for the patch.\n\nOn Wed, May 26, 2021 at 10:10 PM Umang Jain \nwrote:\n\n> Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via\n> IPU3Event. Frame timestamps are helpful to IPA algorithms to\n> convergence, by setting them via IPA stats.\n>\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/ipa/ipu3.mojom | 1 +\n> src/ipa/ipu3/ipu3.cpp | 4 +++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> 3 files changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/ipu3.mojom\n> b/include/libcamera/ipa/ipu3.mojom\n> index 32c046ad..29b4c805 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -21,6 +21,7 @@ enum IPU3Operations {\n> struct IPU3Event {\n> IPU3Operations op;\n> uint32 frame;\n> + int64 frameTimestamp;\n> uint32 bufferId;\n> libcamera.ControlList controls;\n> };\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 769c24d3..581297be 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -53,6 +53,7 @@ private:\n> void processControls(unsigned int frame, const ControlList\n> &controls);\n> void fillParams(unsigned int frame, ipu3_uapi_params *params);\n> void parseStatistics(unsigned int frame,\n> + int64_t frameTimestamp,\n> const ipu3_uapi_stats_3a *stats);\n>\n> void setControls(unsigned int frame);\n> @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> const ipu3_uapi_stats_3a *stats =\n> reinterpret_cast(mem.data());\n>\n> - parseStatistics(event.frame, stats);\n> + parseStatistics(event.frame, event.frameTimestamp, stats);\n> break;\n> }\n> case EventFillParams: {\n> @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame,\n> ipu3_uapi_params *params)\n> }\n>\n> void IPAIPU3::parseStatistics(unsigned int frame,\n> + [[maybe_unused]] int64_t frameTimestamp,\n> [[maybe_unused]] const ipu3_uapi_stats_3a\n> *stats)\n>\n\nWill the passed frameTimestamp be used in a follow up patch?\n\nReviewed-by: Hirokazu Honda \n\n\n> {\n> ControlList ctrls(controls::controls);\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 750880ed..58923bc7 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer\n> *buffer)\n> if (!info)\n> return;\n>\n> + Request *request = info->request;\n> +\n> if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> info->metadataProcessed = true;\n>\n> @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer\n> *buffer)\n> * tryComplete() will delete info if it completes the\n> IPU3Frame.\n> * In that event, we must have obtained the Request before\n> hand.\n> */\n> - Request *request = info->request;\n> -\n> if (frameInfos_.tryComplete(info))\n> pipe_->completeRequest(request);\n>\n> @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer\n> *buffer)\n> ev.op = ipa::ipu3::EventStatReady;\n> ev.frame = info->id;\n> ev.bufferId = info->statBuffer->cookie();\n> + ev.frameTimestamp =\n> request->metadata().get(controls::SensorTimestamp);\n> ipa_->processEvent(ev);\n> }\n>\n> --\n> 2.26.2\n>\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 4FE55BDB80\n\tfor ;\n\tWed, 26 May 2021 13:53:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0D5468925;\n\tWed, 26 May 2021 15:53:41 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A6B1E602AE\n\tfor ;\n\tWed, 26 May 2021 15:53:40 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id jt22so2595231ejb.7\n\tfor ;\n\tWed, 26 May 2021 06:53:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"MRNFyLAC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Wzpa2CS00Dji8x6jfmXuIwIR40E+CyqHaSRe6HSvmFs=;\n\tb=MRNFyLACo8JjpVi9HUpiCm7RFU1gu4l6pmU+TEuC5iQQmWvil/q+BpmxrBgS5yBsay\n\typ1F4542GRuAkxP9oVshhGCTmmRBnlCj6pgzer3+SyVOwoP2lYKe04kHvpeIDHh8nBcF\n\tnA/+gPrleEnn6I3zLYT66yubrUJq6b4x6Todc=","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=Wzpa2CS00Dji8x6jfmXuIwIR40E+CyqHaSRe6HSvmFs=;\n\tb=e+QEuPjdUcsbDnnkxUafRL09U7tePYJFCS7ML+acTFwxYkT6v0tKDVp8wsBSbR/FKt\n\twbE8Zrk5WHq56pSalbObtbC9UCgpkrB2abwpiF+WV4EFkn6INhs/kuXLrlo/+aeiLhvz\n\tjvRRZ/oF9kC5gUbdR5sdjj+yJTd5lnd+ViJ2zwJ/XvXohswO1hbY3ATpGBJFOl0fO0n8\n\t6fg8L2BzOzEC+HQi1m+fN3c+mPf3SidWef9DY+CzWyzfhfwuvZzlCCuUc+ILMr7ENQBk\n\tCYF0TYuREYZMfp3NQN8WQo1gMPhzPLWNqn9129hJHPu1nH/BvfwVikgbzxrrG+ocpeYv\n\tCeqQ==","X-Gm-Message-State":"AOAM533mQhNLXdQfLjUiRXQ9HkOSUWlEbS1rY/T0cYRLrTcNVQbLD2MY\n\tM/yFMFdd/v9aOUs4a2Jp5DMwwT3A447IxGFVqQ14zOquZIg=","X-Google-Smtp-Source":"ABdhPJxth0Du43gbMkRs/FD+H0Y0r5eg3V+dzU1VXb7EYTtW3z1qHcxbLF8sVk5JV7wsZOiGPAy6jX8NMsRkwwrhLdY=","X-Received":"by 2002:a17:907:d1a:: with SMTP id\n\tgn26mr30974104ejc.42.1622037220298; \n\tWed, 26 May 2021 06:53:40 -0700 (PDT)","MIME-Version":"1.0","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>","In-Reply-To":"<20210526131025.675024-3-umang.jain@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Wed, 26 May 2021 22:53:29 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"multipart/alternative; boundary=\"000000000000542a8905c33bf829\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17281,"web_url":"https://patchwork.libcamera.org/comment/17281/","msgid":"","date":"2021-05-26T13:54:32","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:\n> Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via\n> IPU3Event. Frame timestamps are helpful to IPA algorithms to\n> convergence, by setting them via IPA stats.\n\nThis looks good. There's room for improvement, but on top, as it would\nbe too much yak-shaving:\n\n- Using std::chrono types would be nice for timestamps, but that should\n be done throughout libcamera.\n\n- We should move away from IPU3Event, now that the IPA interface is\n specific to each pipeline handler. Instead of a single processEvent()\n method in IPAIPU3Interface, we should have 3 methods, with ad-hoc\n arguments.\n\n> Signed-off-by: Umang Jain \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> include/libcamera/ipa/ipu3.mojom | 1 +\n> src/ipa/ipu3/ipu3.cpp | 4 +++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> 3 files changed, 7 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index 32c046ad..29b4c805 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -21,6 +21,7 @@ enum IPU3Operations {\n> struct IPU3Event {\n> \tIPU3Operations op;\n> \tuint32 frame;\n> +\tint64 frameTimestamp;\n> \tuint32 bufferId;\n> \tlibcamera.ControlList controls;\n> };\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 769c24d3..581297be 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -53,6 +53,7 @@ private:\n> \tvoid processControls(unsigned int frame, const ControlList &controls);\n> \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n> \tvoid parseStatistics(unsigned int frame,\n> +\t\t\t int64_t frameTimestamp,\n> \t\t\t const ipu3_uapi_stats_3a *stats);\n> \n> \tvoid setControls(unsigned int frame);\n> @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> \t\tconst ipu3_uapi_stats_3a *stats =\n> \t\t\treinterpret_cast(mem.data());\n> \n> -\t\tparseStatistics(event.frame, stats);\n> +\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> \t\tbreak;\n> \t}\n> \tcase EventFillParams: {\n> @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> }\n> \n> void IPAIPU3::parseStatistics(unsigned int frame,\n> +\t\t\t [[maybe_unused]] int64_t frameTimestamp,\n> \t\t\t [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> {\n> \tControlList ctrls(controls::controls);\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 750880ed..58923bc7 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> \tif (!info)\n> \t\treturn;\n> \n> +\tRequest *request = info->request;\n> +\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> \t\tinfo->metadataProcessed = true;\n> \n> @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> \t\t * tryComplete() will delete info if it completes the IPU3Frame.\n> \t\t * In that event, we must have obtained the Request before hand.\n> \t\t */\n> -\t\tRequest *request = info->request;\n> -\n> \t\tif (frameInfos_.tryComplete(info))\n> \t\t\tpipe_->completeRequest(request);\n> \n> @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> \tev.op = ipa::ipu3::EventStatReady;\n> \tev.frame = info->id;\n> \tev.bufferId = info->statBuffer->cookie();\n> +\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> \tipa_->processEvent(ev);\n> }\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 CCE1BC3203\n\tfor ;\n\tWed, 26 May 2021 13:54:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B0816892D;\n\tWed, 26 May 2021 15:54:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B4D1C602B1\n\tfor ;\n\tWed, 26 May 2021 15:54:37 +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 462809C7;\n\tWed, 26 May 2021 15:54:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Dldksjfe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622037277;\n\tbh=KoIo2cwP2G89t7g0B+jfeqy1zlTy/R5yukVaBPhZP+g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DldksjfeJqvf7IxD/6onuxovLB3TaxI7csgiLM/NY5Ablk00GIsud5268t2NfSdmj\n\tsT6xkbd6UGqbRmhqg8tnzk38R0EuKIxA1srSbIEtRYrQljQUTJpGhape82kpyjvZ8J\n\tzi03hJGbRd3sLpnA8+BED3ShhXUDLXV2rbepdt2Q=","Date":"Wed, 26 May 2021 16:54:32 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210526131025.675024-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17282,"web_url":"https://patchwork.libcamera.org/comment/17282/","msgid":"","date":"2021-05-26T14:04:10","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Wed, May 26, 2021 at 10:53:29PM +0900, Hirokazu Honda wrote:\n> On Wed, May 26, 2021 at 10:10 PM Umang Jain wrote:\n> \n> > Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via\n> > IPU3Event. Frame timestamps are helpful to IPA algorithms to\n> > convergence, by setting them via IPA stats.\n> >\n> > Signed-off-by: Umang Jain \n> > ---\n> > include/libcamera/ipa/ipu3.mojom | 1 +\n> > src/ipa/ipu3/ipu3.cpp | 4 +++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > 3 files changed, 7 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/ipu3.mojom\n> > b/include/libcamera/ipa/ipu3.mojom\n> > index 32c046ad..29b4c805 100644\n> > --- a/include/libcamera/ipa/ipu3.mojom\n> > +++ b/include/libcamera/ipa/ipu3.mojom\n> > @@ -21,6 +21,7 @@ enum IPU3Operations {\n> > struct IPU3Event {\n> > IPU3Operations op;\n> > uint32 frame;\n> > + int64 frameTimestamp;\n> > uint32 bufferId;\n> > libcamera.ControlList controls;\n> > };\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 769c24d3..581297be 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -53,6 +53,7 @@ private:\n> > void processControls(unsigned int frame, const ControlList &controls);\n> > void fillParams(unsigned int frame, ipu3_uapi_params *params);\n> > void parseStatistics(unsigned int frame,\n> > + int64_t frameTimestamp,\n> > const ipu3_uapi_stats_3a *stats);\n> >\n> > void setControls(unsigned int frame);\n> > @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> > const ipu3_uapi_stats_3a *stats =\n> > reinterpret_cast(mem.data());\n> >\n> > - parseStatistics(event.frame, stats);\n> > + parseStatistics(event.frame, event.frameTimestamp, stats);\n> > break;\n> > }\n> > case EventFillParams: {\n> > @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> > }\n> >\n> > void IPAIPU3::parseStatistics(unsigned int frame,\n> > + [[maybe_unused]] int64_t frameTimestamp,\n> > [[maybe_unused]] const ipu3_uapi_stats_3a*stats)\n> >\n> \n> Will the passed frameTimestamp be used in a follow up patch?\n\nThe IPU3 IPA based on the Intel closed-source library\n(https://git.libcamera.org/libcamera/ipu3-ipa.git/) will use this. I'm\nsure the in-tree open-source IPU3 IPA will follow at some point.\n\n> Reviewed-by: Hirokazu Honda \n> \n> > {\n> > ControlList ctrls(controls::controls);\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 750880ed..58923bc7 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > if (!info)\n> > return;\n> >\n> > + Request *request = info->request;\n> > +\n> > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > info->metadataProcessed = true;\n> >\n> > @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > * tryComplete() will delete info if it completes the IPU3Frame.\n> > * In that event, we must have obtained the Request before hand.\n> > */\n> > - Request *request = info->request;\n> > -\n> > if (frameInfos_.tryComplete(info))\n> > pipe_->completeRequest(request);\n> >\n> > @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > ev.op = ipa::ipu3::EventStatReady;\n> > ev.frame = info->id;\n> > ev.bufferId = info->statBuffer->cookie();\n> > + ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> > ipa_->processEvent(ev);\n> > }\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 7B4F1BDB80\n\tfor ;\n\tWed, 26 May 2021 14:04:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA8F168924;\n\tWed, 26 May 2021 16:04:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 95BF9602AE\n\tfor ;\n\tWed, 26 May 2021 16:04: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 EBD709C7;\n\tWed, 26 May 2021 16:04: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=\"lXApW2Gz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622037856;\n\tbh=8dRP5FWTlTDYLLA4Z6pBkSxNZeZ5ReWpwqd5YZ9Z08Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lXApW2GzbFeP/D/zq3H3XR2ZJ1W6zkYGPmZSoc6cGv+x29+MMU5FzuQ3uhATOtsFp\n\tLBwO1mK0Y9JCpX/7ogoWgGeBb23040inoXivbSAAIaYIfZZwjawZT5MQTPXqx+Uxpe\n\tg32SwH1Dt9SsxOHqs1cD4JiVmqwIIlNUlnoefZXo=","Date":"Wed, 26 May 2021 17:04:10 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17283,"web_url":"https://patchwork.libcamera.org/comment/17283/","msgid":"","date":"2021-05-26T14:24:20","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Wed, May 26, 2021 at 11:04 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Hiro,\n>\n> On Wed, May 26, 2021 at 10:53:29PM +0900, Hirokazu Honda wrote:\n> > On Wed, May 26, 2021 at 10:10 PM Umang Jain wrote:\n> >\n> > > Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via\n> > > IPU3Event. Frame timestamps are helpful to IPA algorithms to\n> > > convergence, by setting them via IPA stats.\n> > >\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > include/libcamera/ipa/ipu3.mojom | 1 +\n> > > src/ipa/ipu3/ipu3.cpp | 4 +++-\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > > 3 files changed, 7 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/ipu3.mojom\n> > > b/include/libcamera/ipa/ipu3.mojom\n> > > index 32c046ad..29b4c805 100644\n> > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > @@ -21,6 +21,7 @@ enum IPU3Operations {\n> > > struct IPU3Event {\n> > > IPU3Operations op;\n> > > uint32 frame;\n> > > + int64 frameTimestamp;\n> > > uint32 bufferId;\n> > > libcamera.ControlList controls;\n> > > };\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index 769c24d3..581297be 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -53,6 +53,7 @@ private:\n> > > void processControls(unsigned int frame, const ControlList\n> &controls);\n> > > void fillParams(unsigned int frame, ipu3_uapi_params *params);\n> > > void parseStatistics(unsigned int frame,\n> > > + int64_t frameTimestamp,\n> > > const ipu3_uapi_stats_3a *stats);\n> > >\n> > > void setControls(unsigned int frame);\n> > > @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> > > const ipu3_uapi_stats_3a *stats =\n> > > reinterpret_cast *>(mem.data());\n> > >\n> > > - parseStatistics(event.frame, stats);\n> > > + parseStatistics(event.frame, event.frameTimestamp,\n> stats);\n> > > break;\n> > > }\n> > > case EventFillParams: {\n> > > @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame,\n> ipu3_uapi_params *params)\n> > > }\n> > >\n> > > void IPAIPU3::parseStatistics(unsigned int frame,\n> > > + [[maybe_unused]] int64_t frameTimestamp,\n> > > [[maybe_unused]] const\n> ipu3_uapi_stats_3a*stats)\n> > >\n> >\n> > Will the passed frameTimestamp be used in a follow up patch?\n>\n> The IPU3 IPA based on the Intel closed-source library\n> (https://git.libcamera.org/libcamera/ipu3-ipa.git/) will use this. I'm\n> sure the in-tree open-source IPU3 IPA will follow at some point.\n>\n>\nAcked.\n\n\n> > Reviewed-by: Hirokazu Honda \n> >\n> > > {\n> > > ControlList ctrls(controls::controls);\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 750880ed..58923bc7 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer\n> *buffer)\n> > > if (!info)\n> > > return;\n> > >\n> > > + Request *request = info->request;\n> > > +\n> > > if (buffer->metadata().status ==\n> FrameMetadata::FrameCancelled) {\n> > > info->metadataProcessed = true;\n> > >\n> > > @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer\n> *buffer)\n> > > * tryComplete() will delete info if it completes the\n> IPU3Frame.\n> > > * In that event, we must have obtained the Request\n> before hand.\n> > > */\n> > > - Request *request = info->request;\n> > > -\n> > > if (frameInfos_.tryComplete(info))\n> > > pipe_->completeRequest(request);\n> > >\n> > > @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer\n> *buffer)\n> > > ev.op = ipa::ipu3::EventStatReady;\n> > > ev.frame = info->id;\n> > > ev.bufferId = info->statBuffer->cookie();\n> > > + ev.frameTimestamp =\n> request->metadata().get(controls::SensorTimestamp);\n> > > ipa_->processEvent(ev);\n> > > }\n> > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 689CEC3203\n\tfor ;\n\tWed, 26 May 2021 14:24:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB1566891D;\n\tWed, 26 May 2021 16:24:32 +0200 (CEST)","from mail-ed1-x52c.google.com (mail-ed1-x52c.google.com\n\t[IPv6:2a00:1450:4864:20::52c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F15C602AB\n\tfor ;\n\tWed, 26 May 2021 16:24:31 +0200 (CEST)","by mail-ed1-x52c.google.com with SMTP id o5so1759930edc.5\n\tfor ;\n\tWed, 26 May 2021 07:24:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"QDCXR5gK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=jVQEn+N71nX5O23h1p0uGb/hTn7U/iuPhc+XC6a2tbs=;\n\tb=QDCXR5gK7KxIzKheh+eKyO7E+QZccVsc0LNDrPShiAoGVMp66S399i2f6mhNzkYNt4\n\twHLdVeQK2kpBAY0wqV74JUwSi7kYae02m+AwlEyY+RXqGv8VJ0Vs+Fx9ZgQ8R6XkRcQZ\n\tuj7N6jjFQ6stvrT1StKRqjQRhTPdRZtf/UCF0=","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=jVQEn+N71nX5O23h1p0uGb/hTn7U/iuPhc+XC6a2tbs=;\n\tb=C+na1dVfirXqMq/mmzYNNk72aERDI/hnCJK4FueczgOBSwlxrCv9IDD9P6TFV9VcBI\n\tmMktThNMkCxo6fdbh4H3wGV/O3kOaagOUeEHcqtnZqrpcvBwPzYRBBNjfrzAQACiNb9g\n\tosnOBG1hQyQAPaEpZEPYuzwslLaa1kXgC+CYZkb+xvtXcp03oYnoepwyrH9Mim29skvr\n\t2btIVeJVXh5D932IpXmZikWme7yEySj956ivz+IWBYKzEVj7AYr8+eY16flE1E3KnC6m\n\t3EN0TqyVAOu19YqAnPEBA+oKuzT2yTgzkb/izVBvYjljASfhZeIEp6ufgZDNwCavYk/2\n\t1shw==","X-Gm-Message-State":"AOAM532iGXH4+kiji3QDe7QqX9UDxUkf6Y2+KbFwLXHuxHmTb2JAoocz\n\tT/jq/JJIo0UhUje45ifkiBY7TXRrB5G6qOR49c7V8Q==","X-Google-Smtp-Source":"ABdhPJzWtMpCit9668kMdL9d9x5RPFY8TdeI8TQFPwrY0ujVxT48cJz15zNkG1Q1NUS6hWtx4flmiz8N2XVIJcIyHEU=","X-Received":"by 2002:a05:6402:50c6:: with SMTP id\n\th6mr38100781edb.327.1622039071011; \n\tWed, 26 May 2021 07:24:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 26 May 2021 23:24:20 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000a3c66f05c33c663e\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17291,"web_url":"https://patchwork.libcamera.org/comment/17291/","msgid":"<20210526200110.jhl7i47aefn56q4a@uno.localdomain>","date":"2021-05-26T20:01:10","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi\n\nOn Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:\n> > Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via\n> > IPU3Event. Frame timestamps are helpful to IPA algorithms to\n> > convergence, by setting them via IPA stats.\n>\n> This looks good. There's room for improvement, but on top, as it would\n> be too much yak-shaving:\n>\n> - Using std::chrono types would be nice for timestamps, but that should\n> be done throughout libcamera.\n\nJust to mention that Nuash's utils::Duration will be a nice fit to\nreplace all the custom timestampings in the library\n\nFor the patch:\n- We create frameTimestamp by copying SensorTimestamp. Would it be\n better to use the same name ?\n\n>\n> - We should move away from IPU3Event, now that the IPA interface is\n> specific to each pipeline handler. Instead of a single processEvent()\n> method in IPAIPU3Interface, we should have 3 methods, with ad-hoc\n> arguments.\n>\n> > Signed-off-by: Umang Jain \n>\n> Reviewed-by: Laurent Pinchart \n>\n> > ---\n> > include/libcamera/ipa/ipu3.mojom | 1 +\n> > src/ipa/ipu3/ipu3.cpp | 4 +++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > 3 files changed, 7 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > index 32c046ad..29b4c805 100644\n> > --- a/include/libcamera/ipa/ipu3.mojom\n> > +++ b/include/libcamera/ipa/ipu3.mojom\n> > @@ -21,6 +21,7 @@ enum IPU3Operations {\n> > struct IPU3Event {\n> > \tIPU3Operations op;\n> > \tuint32 frame;\n> > +\tint64 frameTimestamp;\n> > \tuint32 bufferId;\n> > \tlibcamera.ControlList controls;\n> > };\n> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > index 769c24d3..581297be 100644\n> > --- a/src/ipa/ipu3/ipu3.cpp\n> > +++ b/src/ipa/ipu3/ipu3.cpp\n> > @@ -53,6 +53,7 @@ private:\n> > \tvoid processControls(unsigned int frame, const ControlList &controls);\n> > \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n> > \tvoid parseStatistics(unsigned int frame,\n> > +\t\t\t int64_t frameTimestamp,\n> > \t\t\t const ipu3_uapi_stats_3a *stats);\n> >\n> > \tvoid setControls(unsigned int frame);\n> > @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> > \t\tconst ipu3_uapi_stats_3a *stats =\n> > \t\t\treinterpret_cast(mem.data());\n> >\n> > -\t\tparseStatistics(event.frame, stats);\n> > +\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> > \t\tbreak;\n> > \t}\n> > \tcase EventFillParams: {\n> > @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> > }\n> >\n> > void IPAIPU3::parseStatistics(unsigned int frame,\n> > +\t\t\t [[maybe_unused]] int64_t frameTimestamp,\n> > \t\t\t [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> > {\n> > \tControlList ctrls(controls::controls);\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 750880ed..58923bc7 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > \tif (!info)\n> > \t\treturn;\n> >\n> > +\tRequest *request = info->request;\n> > +\n> > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > \t\tinfo->metadataProcessed = true;\n> >\n> > @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > \t\t * tryComplete() will delete info if it completes the IPU3Frame.\n> > \t\t * In that event, we must have obtained the Request before hand.\n> > \t\t */\n> > -\t\tRequest *request = info->request;\n> > -\n> > \t\tif (frameInfos_.tryComplete(info))\n> > \t\t\tpipe_->completeRequest(request);\n> >\n> > @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > \tev.op = ipa::ipu3::EventStatReady;\n> > \tev.frame = info->id;\n> > \tev.bufferId = info->statBuffer->cookie();\n> > +\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> > \tipa_->processEvent(ev);\n> > }\n> >\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 4B9A2BDB80\n\tfor ;\n\tWed, 26 May 2021 20:00:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8740668923;\n\tWed, 26 May 2021 22:00:26 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46074602B1\n\tfor ;\n\tWed, 26 May 2021 22:00:25 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 526CA40003;\n\tWed, 26 May 2021 20:00:24 +0000 (UTC)"],"Date":"Wed, 26 May 2021 22:01:10 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210526200110.jhl7i47aefn56q4a@uno.localdomain>","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17297,"web_url":"https://patchwork.libcamera.org/comment/17297/","msgid":"","date":"2021-05-26T23:45:40","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, May 26, 2021 at 10:01:10PM +0200, Jacopo Mondi wrote:\n> On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote:\n> > On Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:\n> > > Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via\n> > > IPU3Event. Frame timestamps are helpful to IPA algorithms to\n> > > convergence, by setting them via IPA stats.\n> >\n> > This looks good. There's room for improvement, but on top, as it would\n> > be too much yak-shaving:\n> >\n> > - Using std::chrono types would be nice for timestamps, but that should\n> > be done throughout libcamera.\n> \n> Just to mention that Nuash's utils::Duration will be a nice fit to\n> replace all the custom timestampings in the library\n\nNote that this is a timestamp, not a duration. It should thus be\nmodelled with a std::chrono::time_point<> instance.\n\n> For the patch:\n> - We create frameTimestamp by copying SensorTimestamp. Would it be\n> better to use the same name ?\n\nI don't have a strong preference here, but note that we fill the\nSensorTimestamp metadata with an approximation of the sensor timestamp,\nwhich is the CIO2 frame timestamp. We could consider that the IPU3 IPA\nAPI would be less misleading with a name that reflects the actual\ncontent of the value.\n\nThis being said, I'd like to see at some point improvements to the\ntimestamp approximation, and I don't think we should then rename the\nfield in the IPU3Event structure, so maybe we can already call it\nsensorTimestamp.\n\n> > - We should move away from IPU3Event, now that the IPA interface is\n> > specific to each pipeline handler. Instead of a single processEvent()\n> > method in IPAIPU3Interface, we should have 3 methods, with ad-hoc\n> > arguments.\n> >\n> > > Signed-off-by: Umang Jain \n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > ---\n> > > include/libcamera/ipa/ipu3.mojom | 1 +\n> > > src/ipa/ipu3/ipu3.cpp | 4 +++-\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > > 3 files changed, 7 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > > index 32c046ad..29b4c805 100644\n> > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > @@ -21,6 +21,7 @@ enum IPU3Operations {\n> > > struct IPU3Event {\n> > > \tIPU3Operations op;\n> > > \tuint32 frame;\n> > > +\tint64 frameTimestamp;\n> > > \tuint32 bufferId;\n> > > \tlibcamera.ControlList controls;\n> > > };\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index 769c24d3..581297be 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -53,6 +53,7 @@ private:\n> > > \tvoid processControls(unsigned int frame, const ControlList &controls);\n> > > \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n> > > \tvoid parseStatistics(unsigned int frame,\n> > > +\t\t\t int64_t frameTimestamp,\n> > > \t\t\t const ipu3_uapi_stats_3a *stats);\n> > >\n> > > \tvoid setControls(unsigned int frame);\n> > > @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> > > \t\tconst ipu3_uapi_stats_3a *stats =\n> > > \t\t\treinterpret_cast(mem.data());\n> > >\n> > > -\t\tparseStatistics(event.frame, stats);\n> > > +\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> > > \t\tbreak;\n> > > \t}\n> > > \tcase EventFillParams: {\n> > > @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> > > }\n> > >\n> > > void IPAIPU3::parseStatistics(unsigned int frame,\n> > > +\t\t\t [[maybe_unused]] int64_t frameTimestamp,\n> > > \t\t\t [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> > > {\n> > > \tControlList ctrls(controls::controls);\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 750880ed..58923bc7 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > > \tif (!info)\n> > > \t\treturn;\n> > >\n> > > +\tRequest *request = info->request;\n> > > +\n> > > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > \t\tinfo->metadataProcessed = true;\n> > >\n> > > @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > > \t\t * tryComplete() will delete info if it completes the IPU3Frame.\n> > > \t\t * In that event, we must have obtained the Request before hand.\n> > > \t\t */\n> > > -\t\tRequest *request = info->request;\n> > > -\n> > > \t\tif (frameInfos_.tryComplete(info))\n> > > \t\t\tpipe_->completeRequest(request);\n> > >\n> > > @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > > \tev.op = ipa::ipu3::EventStatReady;\n> > > \tev.frame = info->id;\n> > > \tev.bufferId = info->statBuffer->cookie();\n> > > +\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> > > \tipa_->processEvent(ev);\n> > > }\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 4B830BDB80\n\tfor ;\n\tWed, 26 May 2021 23:45:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3C1A68923;\n\tThu, 27 May 2021 01:45:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6FEA46050E\n\tfor ;\n\tThu, 27 May 2021 01:45:46 +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 DB5E0AEF;\n\tThu, 27 May 2021 01:45:45 +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=\"uTce2dwu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622072746;\n\tbh=ELHfMxGT8XOXPJP4sINJAi6oiecVBhp2vRYc+mvMHHQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uTce2dwuSXJJfYaQ5WeBisRR6V9UjmWDdx6W0WywLSj82Vs1CeuQwQbic8AqZhq8u\n\tjk8ZHmPiqEaa+cirg4wRV4JrsrX/rtXvH2AtLatBzTc0ZPqul32V/9FtM6wlpsHzmA\n\tt4avWGap4pkeoXNMa8hJCfIYZKB8rdex2CjzNwn0=","Date":"Thu, 27 May 2021 02:45:40 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>\n\t\n\t<20210526200110.jhl7i47aefn56q4a@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210526200110.jhl7i47aefn56q4a@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17304,"web_url":"https://patchwork.libcamera.org/comment/17304/","msgid":"<989dd5bc-76ae-549c-9df8-719838561c2d@ideasonboard.com>","date":"2021-05-27T03:58:02","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 5/27/21 1:31 AM, Jacopo Mondi wrote:\n> Hi\n>\n> On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote:\n>> Hi Umang,\n>>\n>> Thank you for the patch.\n>>\n>> On Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:\n>>> Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via\n>>> IPU3Event. Frame timestamps are helpful to IPA algorithms to\n>>> convergence, by setting them via IPA stats.\n>> This looks good. There's room for improvement, but on top, as it would\n>> be too much yak-shaving:\n>>\n>> - Using std::chrono types would be nice for timestamps, but that should\n>> be done throughout libcamera.\n> Just to mention that Nuash's utils::Duration will be a nice fit to\n> replace all the custom timestampings in the library\n>\n> For the patch:\n> - We create frameTimestamp by copying SensorTimestamp. Would it be\n> better to use the same name ?\n>\nThe IPA is expecting a frame timestamp on it's side. It's entirely upto \nthe PH to pass in, whatever it thinks closest as the timestamp of the \nframe. In this case, that's SensorTimestamp. I don't support that the \nnaming should be changed just because we are passing in Sensor's \ntimestamp. I am open to suggestions nevertheless, maybe an explicit \ncomment on PH side :\n\n     /* As of now, SensorTimestamp is the best approximation for the \nframe-timestamp */\n\nInterestingly raspberry-pi IPA also does similar,\n\nsrc/ipa/raspberrypi/raspberrypi.cpp:911:        int64_t frameTimestamp = \ndata.controls.get(controls::SensorTimestamp);\n\n>> - We should move away from IPU3Event, now that the IPA interface is\n>> specific to each pipeline handler. Instead of a single processEvent()\n>> method in IPAIPU3Interface, we should have 3 methods, with ad-hoc\n>> arguments.\n>>\n>>> Signed-off-by: Umang Jain \n>> Reviewed-by: Laurent Pinchart \n>>\n>>> ---\n>>> include/libcamera/ipa/ipu3.mojom | 1 +\n>>> src/ipa/ipu3/ipu3.cpp | 4 +++-\n>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n>>> 3 files changed, 7 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>>> index 32c046ad..29b4c805 100644\n>>> --- a/include/libcamera/ipa/ipu3.mojom\n>>> +++ b/include/libcamera/ipa/ipu3.mojom\n>>> @@ -21,6 +21,7 @@ enum IPU3Operations {\n>>> struct IPU3Event {\n>>> \tIPU3Operations op;\n>>> \tuint32 frame;\n>>> +\tint64 frameTimestamp;\n>>> \tuint32 bufferId;\n>>> \tlibcamera.ControlList controls;\n>>> };\n>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>> index 769c24d3..581297be 100644\n>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>> @@ -53,6 +53,7 @@ private:\n>>> \tvoid processControls(unsigned int frame, const ControlList &controls);\n>>> \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n>>> \tvoid parseStatistics(unsigned int frame,\n>>> +\t\t\t int64_t frameTimestamp,\n>>> \t\t\t const ipu3_uapi_stats_3a *stats);\n>>>\n>>> \tvoid setControls(unsigned int frame);\n>>> @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n>>> \t\tconst ipu3_uapi_stats_3a *stats =\n>>> \t\t\treinterpret_cast(mem.data());\n>>>\n>>> -\t\tparseStatistics(event.frame, stats);\n>>> +\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n>>> \t\tbreak;\n>>> \t}\n>>> \tcase EventFillParams: {\n>>> @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n>>> }\n>>>\n>>> void IPAIPU3::parseStatistics(unsigned int frame,\n>>> +\t\t\t [[maybe_unused]] int64_t frameTimestamp,\n>>> \t\t\t [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n>>> {\n>>> \tControlList ctrls(controls::controls);\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index 750880ed..58923bc7 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>> \tif (!info)\n>>> \t\treturn;\n>>>\n>>> +\tRequest *request = info->request;\n>>> +\n>>> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n>>> \t\tinfo->metadataProcessed = true;\n>>>\n>>> @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>> \t\t * tryComplete() will delete info if it completes the IPU3Frame.\n>>> \t\t * In that event, we must have obtained the Request before hand.\n>>> \t\t */\n>>> -\t\tRequest *request = info->request;\n>>> -\n>>> \t\tif (frameInfos_.tryComplete(info))\n>>> \t\t\tpipe_->completeRequest(request);\n>>>\n>>> @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n>>> \tev.op = ipa::ipu3::EventStatReady;\n>>> \tev.frame = info->id;\n>>> \tev.bufferId = info->statBuffer->cookie();\n>>> +\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n>>> \tipa_->processEvent(ev);\n>>> }\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 BFBA1C3203\n\tfor ;\n\tThu, 27 May 2021 03:58:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1716068924;\n\tThu, 27 May 2021 05:58: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 8411C6891F\n\tfor ;\n\tThu, 27 May 2021 05:58:15 +0200 (CEST)","from localhost.localdomain (unknown [103.251.226.180])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 32D028DE;\n\tThu, 27 May 2021 05:58:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Iy1W488d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622087895;\n\tbh=hdE0/RbOlOCjl/bOyPJ8RrScNUEf04qNPc96o6W2jGc=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Iy1W488d2ZZcXMIM1nvpqpdk2W9Ysu0Ab2HvCsZVicRengja2dErT9da6PavTOf0C\n\ttwKAp7NY6oHMhsvgsgq+YqWJuw1KSHo6fdApiUIPDn5g/2kGRNeWkb5LziVBLtPzFk\n\tdJ3Vk4D8RnIt2e4LnZ/jfTmsmPdYbUsizZYWr5ow=","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>\n\t\n\t<20210526200110.jhl7i47aefn56q4a@uno.localdomain>","From":"Umang Jain ","Message-ID":"<989dd5bc-76ae-549c-9df8-719838561c2d@ideasonboard.com>","Date":"Thu, 27 May 2021 09:28:02 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.11.0","MIME-Version":"1.0","In-Reply-To":"<20210526200110.jhl7i47aefn56q4a@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17336,"web_url":"https://patchwork.libcamera.org/comment/17336/","msgid":"","date":"2021-05-27T16:10:10","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Thu, May 27, 2021 at 09:28:02AM +0530, Umang Jain wrote:\n> On 5/27/21 1:31 AM, Jacopo Mondi wrote:\n> > On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote:\n> >> On Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:\n> >>> Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via\n> >>> IPU3Event. Frame timestamps are helpful to IPA algorithms to\n> >>> convergence, by setting them via IPA stats.\n> >> This looks good. There's room for improvement, but on top, as it would\n> >> be too much yak-shaving:\n> >>\n> >> - Using std::chrono types would be nice for timestamps, but that should\n> >> be done throughout libcamera.\n> >\n> > Just to mention that Nuash's utils::Duration will be a nice fit to\n> > replace all the custom timestampings in the library\n> >\n> > For the patch:\n> > - We create frameTimestamp by copying SensorTimestamp. Would it be\n> > better to use the same name ?\n>\n> The IPA is expecting a frame timestamp on it's side. It's entirely upto \n> the PH to pass in, whatever it thinks closest as the timestamp of the \n> frame. In this case, that's SensorTimestamp. I don't support that the \n> naming should be changed just because we are passing in Sensor's \n> timestamp. I am open to suggestions nevertheless, maybe an explicit \n> comment on PH side :\n> \n>     /* As of now, SensorTimestamp is the best approximation for the \n> frame-timestamp */\n> \n> Interestingly raspberry-pi IPA also does similar,\n> \n> src/ipa/raspberrypi/raspberrypi.cpp:911:        int64_t frameTimestamp = \n> data.controls.get(controls::SensorTimestamp);\n\nThe RPi IPA uses the timestamp to implement rate-limiting of the\nalgorithms. I'd expect IPAs to care more about the timestamp delta\nbetween frames than the absolute timestamp, so from that point of view\nit doesn't matter much how we approximate the timestamp, as long as we\ndo so consistently (minimizing jitter would however be important).\n\n> >> - We should move away from IPU3Event, now that the IPA interface is\n> >> specific to each pipeline handler. Instead of a single processEvent()\n> >> method in IPAIPU3Interface, we should have 3 methods, with ad-hoc\n> >> arguments.\n> >>\n> >>> Signed-off-by: Umang Jain \n> >> Reviewed-by: Laurent Pinchart \n> >>\n> >>> ---\n> >>> include/libcamera/ipa/ipu3.mojom | 1 +\n> >>> src/ipa/ipu3/ipu3.cpp | 4 +++-\n> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> >>> 3 files changed, 7 insertions(+), 3 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> >>> index 32c046ad..29b4c805 100644\n> >>> --- a/include/libcamera/ipa/ipu3.mojom\n> >>> +++ b/include/libcamera/ipa/ipu3.mojom\n> >>> @@ -21,6 +21,7 @@ enum IPU3Operations {\n> >>> struct IPU3Event {\n> >>> \tIPU3Operations op;\n> >>> \tuint32 frame;\n> >>> +\tint64 frameTimestamp;\n> >>> \tuint32 bufferId;\n> >>> \tlibcamera.ControlList controls;\n> >>> };\n> >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>> index 769c24d3..581297be 100644\n> >>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>> @@ -53,6 +53,7 @@ private:\n> >>> \tvoid processControls(unsigned int frame, const ControlList &controls);\n> >>> \tvoid fillParams(unsigned int frame, ipu3_uapi_params *params);\n> >>> \tvoid parseStatistics(unsigned int frame,\n> >>> +\t\t\t int64_t frameTimestamp,\n> >>> \t\t\t const ipu3_uapi_stats_3a *stats);\n> >>>\n> >>> \tvoid setControls(unsigned int frame);\n> >>> @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> >>> \t\tconst ipu3_uapi_stats_3a *stats =\n> >>> \t\t\treinterpret_cast(mem.data());\n> >>>\n> >>> -\t\tparseStatistics(event.frame, stats);\n> >>> +\t\tparseStatistics(event.frame, event.frameTimestamp, stats);\n> >>> \t\tbreak;\n> >>> \t}\n> >>> \tcase EventFillParams: {\n> >>> @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >>> }\n> >>>\n> >>> void IPAIPU3::parseStatistics(unsigned int frame,\n> >>> +\t\t\t [[maybe_unused]] int64_t frameTimestamp,\n> >>> \t\t\t [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> >>> {\n> >>> \tControlList ctrls(controls::controls);\n> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> index 750880ed..58923bc7 100644\n> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> >>> \tif (!info)\n> >>> \t\treturn;\n> >>>\n> >>> +\tRequest *request = info->request;\n> >>> +\n> >>> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> >>> \t\tinfo->metadataProcessed = true;\n> >>>\n> >>> @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> >>> \t\t * tryComplete() will delete info if it completes the IPU3Frame.\n> >>> \t\t * In that event, we must have obtained the Request before hand.\n> >>> \t\t */\n> >>> -\t\tRequest *request = info->request;\n> >>> -\n> >>> \t\tif (frameInfos_.tryComplete(info))\n> >>> \t\t\tpipe_->completeRequest(request);\n> >>>\n> >>> @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> >>> \tev.op = ipa::ipu3::EventStatReady;\n> >>> \tev.frame = info->id;\n> >>> \tev.bufferId = info->statBuffer->cookie();\n> >>> +\tev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> >>> \tipa_->processEvent(ev);\n> >>> }\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 7E41BBDB80\n\tfor ;\n\tThu, 27 May 2021 16:10:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D3660602AE;\n\tThu, 27 May 2021 18:10: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 9AFB0602AA\n\tfor ;\n\tThu, 27 May 2021 18:10: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 07650163F;\n\tThu, 27 May 2021 18:10: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=\"r6YvHpj+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622131816;\n\tbh=W/dOr7SVsJlhsy6UUM7qRIySh+c+4u7JGUNsxmGTw10=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r6YvHpj+6uKAUtiP2Bo22G7/EkLrCvXKLxcBwrfB1Hsr++eLVhEyOzpoQK3ZBbNqa\n\tO2m/UCu/9U+qH3cQNnxGej2MfuDcJN13oJNSzfISSl8+Y5qeOIHAHdr1HnuRDYX6Mo\n\tF6b8f8HZsp7q7hlyP+SB5MTYlLGdzePknJepBRs0=","Date":"Thu, 27 May 2021 19:10:10 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>\n\t\n\t<20210526200110.jhl7i47aefn56q4a@uno.localdomain>\n\t<989dd5bc-76ae-549c-9df8-719838561c2d@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<989dd5bc-76ae-549c-9df8-719838561c2d@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17342,"web_url":"https://patchwork.libcamera.org/comment/17342/","msgid":"","date":"2021-05-28T07:55:07","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 27 May 2021 at 17:10, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Umang,\n>\n> On Thu, May 27, 2021 at 09:28:02AM +0530, Umang Jain wrote:\n> > On 5/27/21 1:31 AM, Jacopo Mondi wrote:\n> > > On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote:\n> > >> On Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:\n> > >>> Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via\n> > >>> IPU3Event. Frame timestamps are helpful to IPA algorithms to\n> > >>> convergence, by setting them via IPA stats.\n> > >> This looks good. There's room for improvement, but on top, as it would\n> > >> be too much yak-shaving:\n> > >>\n> > >> - Using std::chrono types would be nice for timestamps, but that\n> should\n> > >> be done throughout libcamera.\n> > >\n> > > Just to mention that Nuash's utils::Duration will be a nice fit to\n> > > replace all the custom timestampings in the library\n> > >\n> > > For the patch:\n> > > - We create frameTimestamp by copying SensorTimestamp. Would it be\n> > > better to use the same name ?\n> >\n> > The IPA is expecting a frame timestamp on it's side. It's entirely upto\n> > the PH to pass in, whatever it thinks closest as the timestamp of the\n> > frame. In this case, that's SensorTimestamp. I don't support that the\n> > naming should be changed just because we are passing in Sensor's\n> > timestamp. I am open to suggestions nevertheless, maybe an explicit\n> > comment on PH side :\n> >\n> > /* As of now, SensorTimestamp is the best approximation for the\n> > frame-timestamp */\n> >\n> > Interestingly raspberry-pi IPA also does similar,\n> >\n> > src/ipa/raspberrypi/raspberrypi.cpp:911: int64_t frameTimestamp =\n> > data.controls.get(controls::SensorTimestamp);\n>\n> The RPi IPA uses the timestamp to implement rate-limiting of the\n> algorithms. I'd expect IPAs to care more about the timestamp delta\n> between frames than the absolute timestamp, so from that point of view\n> it doesn't matter much how we approximate the timestamp, as long as we\n> do so consistently (minimizing jitter would however be important).\n>\n\nPardon the silly question, but what is the difference between \"frame\ntimestamp\"\nand \"sensor timestamp\" in this context?\n\n\n>\n> > >> - We should move away from IPU3Event, now that the IPA interface is\n> > >> specific to each pipeline handler. Instead of a single\n> processEvent()\n> > >> method in IPAIPU3Interface, we should have 3 methods, with ad-hoc\n> > >> arguments.\n> > >>\n> > >>> Signed-off-by: Umang Jain \n> > >> Reviewed-by: Laurent Pinchart \n> > >>\n> > >>> ---\n> > >>> include/libcamera/ipa/ipu3.mojom | 1 +\n> > >>> src/ipa/ipu3/ipu3.cpp | 4 +++-\n> > >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > >>> 3 files changed, 7 insertions(+), 3 deletions(-)\n> > >>>\n> > >>> diff --git a/include/libcamera/ipa/ipu3.mojom\n> b/include/libcamera/ipa/ipu3.mojom\n> > >>> index 32c046ad..29b4c805 100644\n> > >>> --- a/include/libcamera/ipa/ipu3.mojom\n> > >>> +++ b/include/libcamera/ipa/ipu3.mojom\n> > >>> @@ -21,6 +21,7 @@ enum IPU3Operations {\n> > >>> struct IPU3Event {\n> > >>> IPU3Operations op;\n> > >>> uint32 frame;\n> > >>> + int64 frameTimestamp;\n> > >>> uint32 bufferId;\n> > >>> libcamera.ControlList controls;\n> > >>> };\n> > >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > >>> index 769c24d3..581297be 100644\n> > >>> --- a/src/ipa/ipu3/ipu3.cpp\n> > >>> +++ b/src/ipa/ipu3/ipu3.cpp\n> > >>> @@ -53,6 +53,7 @@ private:\n> > >>> void processControls(unsigned int frame, const ControlList\n> &controls);\n> > >>> void fillParams(unsigned int frame, ipu3_uapi_params\n> *params);\n> > >>> void parseStatistics(unsigned int frame,\n> > >>> + int64_t frameTimestamp,\n> > >>> const ipu3_uapi_stats_3a *stats);\n> > >>>\n> > >>> void setControls(unsigned int frame);\n> > >>> @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event\n> &event)\n> > >>> const ipu3_uapi_stats_3a *stats =\n> > >>> reinterpret_cast *>(mem.data());\n> > >>>\n> > >>> - parseStatistics(event.frame, stats);\n> > >>> + parseStatistics(event.frame, event.frameTimestamp, stats);\n> > >>> break;\n> > >>> }\n> > >>> case EventFillParams: {\n> > >>> @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame,\n> ipu3_uapi_params *params)\n> > >>> }\n> > >>>\n> > >>> void IPAIPU3::parseStatistics(unsigned int frame,\n> > >>> + [[maybe_unused]] int64_t frameTimestamp,\n> > >>> [[maybe_unused]] const\n> ipu3_uapi_stats_3a *stats)\n> > >>> {\n> > >>> ControlList ctrls(controls::controls);\n> > >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> index 750880ed..58923bc7 100644\n> > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> @@ -1357,6 +1357,8 @@ void\n> IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > >>> if (!info)\n> > >>> return;\n> > >>>\n> > >>> + Request *request = info->request;\n> > >>> +\n> > >>> if (buffer->metadata().status ==\n> FrameMetadata::FrameCancelled) {\n> > >>> info->metadataProcessed = true;\n> > >>>\n> > >>> @@ -1364,8 +1366,6 @@ void\n> IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > >>> * tryComplete() will delete info if it completes\n> the IPU3Frame.\n> > >>> * In that event, we must have obtained the\n> Request before hand.\n> > >>> */\n> > >>> - Request *request = info->request;\n> > >>> -\n> > >>> if (frameInfos_.tryComplete(info))\n> > >>> pipe_->completeRequest(request);\n> > >>>\n> > >>> @@ -1376,6 +1376,7 @@ void\n> IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > >>> ev.op = ipa::ipu3::EventStatReady;\n> > >>> ev.frame = info->id;\n> > >>> ev.bufferId = info->statBuffer->cookie();\n> > >>> + ev.frameTimestamp =\n> request->metadata().get(controls::SensorTimestamp);\n> > >>> ipa_->processEvent(ev);\n> > >>> }\n> > >>>\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 28DEFC3205\n\tfor ;\n\tFri, 28 May 2021 07:55:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9CD146891E;\n\tFri, 28 May 2021 09:55:26 +0200 (CEST)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A9706891E\n\tfor ;\n\tFri, 28 May 2021 09:55:24 +0200 (CEST)","by mail-lf1-x12e.google.com with SMTP id x38so3985453lfa.10\n\tfor ;\n\tFri, 28 May 2021 00:55:24 -0700 (PDT)"],"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=\"ZE4LrV0D\"; 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=y4/LI/J2xrVsqyMVdMyUfmqkdT/tRzQqPEXZcMyR468=;\n\tb=ZE4LrV0D4DglocXmnE/oN+u6eQvdIg7puobB8LZ95Luj9Ek4RGYoHNq7AmEZHkSIib\n\tk6D1syp/KmdA0syu9G3zVJmuTW0oV5hLQrHp+t9XDscGx+R6mmYwTlFoI+8V9VQ1ko8f\n\tutK5cdxsNPNYn6IoZSjBuiz9yNBOa29m2klGR/EWEGlq8RIIq6xkjGwEWZnjcJ8Wgrmh\n\tuSTUWX5CvjN8VAFfKzYJYHZXuzX0mgRr2s8yeJZZFsfhJ/1kC2xejhI2ALOOBVitfL5w\n\tCpGWtNPb3TJLmQnfGkmlzrES/9IQrk9r1diGm4NgJUTjtbhuqRKsI3bEQIhRB3m0E7I6\n\twxbw==","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=y4/LI/J2xrVsqyMVdMyUfmqkdT/tRzQqPEXZcMyR468=;\n\tb=uILcHtqqPDWiPiDkEcXDaG1ClNjtRZkIiImFnIYFyOiyuyGe3WX/hKKQo267Vr3i10\n\tmY8Mg2Ah97Imn03SL/qbVCtBKo2+XCDpZGv5REbF86JgDimyOR9stTG3npU+kNMFotVx\n\tpRiHiwzcXYJqd8PpOR3mFtffV4cP9aVmzQGfNduqDtAF41hf1H47ejNt/j7uxiGOrXYQ\n\tz+4e9HthRr/ToK9GouubgILxgvXSmOpTxuXUFRl3vd+iP7tvEpyvdSVecqRvf7ZUwvw6\n\tZkTE2MppDw1V+lLHO76m/fn/ytHMoCNyYchm1JDHEIIOIf08d/qcWpg+HeA5OxLw8Lks\n\tQqBw==","X-Gm-Message-State":"AOAM530381uWqgn3jp9cTmzz3vlR77ptYDYB8UGb2tgQLSlULYIMPKSv\n\t/hZNmXCPvwy/WfloojG/jjYOL1fGylrdcB63WPBfT67AkbI=","X-Google-Smtp-Source":"ABdhPJzzVsNxoHvHN0IL56JTeoKHFNxa41gQmzv969+aZK8Ep4+HCf8/d93+BURQ8lXBGHWt4JdgnxkxOy8R32Df9MI=","X-Received":"by 2002:a05:6512:b17:: with SMTP id\n\tw23mr4846470lfu.133.1622188523804; \n\tFri, 28 May 2021 00:55:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>\n\t\n\t<20210526200110.jhl7i47aefn56q4a@uno.localdomain>\n\t<989dd5bc-76ae-549c-9df8-719838561c2d@ideasonboard.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 28 May 2021 08:55:07 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000b86bff05c35f32df\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17346,"web_url":"https://patchwork.libcamera.org/comment/17346/","msgid":"","date":"2021-05-29T23:18:26","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 28/05/2021 08:55, Naushir Patuck wrote:\n> On Thu, 27 May 2021 at 17:10, Laurent Pinchart\n> > wrote:\n> \n> Hi Umang,\n> \n> On Thu, May 27, 2021 at 09:28:02AM +0530, Umang Jain wrote:\n> > On 5/27/21 1:31 AM, Jacopo Mondi wrote:\n> > > On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote:\n> > >> On Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:\n> > >>> Pass in frame timestamps from IPU3 pipeline handler to IPU3\n> IPA via\n> > >>> IPU3Event. Frame timestamps are helpful to IPA algorithms to\n> > >>> convergence, by setting them via IPA stats.\n> > >> This looks good. There's room for improvement, but on top, as\n> it would\n> > >> be too much yak-shaving:\n> > >>\n> > >> - Using std::chrono types would be nice for timestamps, but\n> that should\n> > >>    be done throughout libcamera.\n> > >\n> > > Just to mention that Nuash's utils::Duration will be a nice fit to\n> > > replace all the custom timestampings in the library\n> > >\n> > > For the patch:\n> > > - We create frameTimestamp by copying SensorTimestamp. Would it be\n> > >    better to use the same name ?\n> >\n> > The IPA is expecting a frame timestamp on it's side. It's entirely\n> upto\n> > the PH to pass in, whatever it thinks closest as the timestamp of the\n> > frame. In this case, that's SensorTimestamp. I don't support that the\n> > naming should be changed just because we are passing in Sensor's\n> > timestamp. I am open to suggestions nevertheless, maybe an explicit\n> > comment on PH side :\n> >\n> >      /* As of now, SensorTimestamp is the best approximation for the\n> > frame-timestamp */\n> >\n> > Interestingly raspberry-pi IPA also does similar,\n> >\n> > src/ipa/raspberrypi/raspberrypi.cpp:911:        int64_t\n> frameTimestamp =\n> > data.controls.get(controls::SensorTimestamp);\n> \n> The RPi IPA uses the timestamp to implement rate-limiting of the\n> algorithms. I'd expect IPAs to care more about the timestamp delta\n> between frames than the absolute timestamp, so from that point of view\n> it doesn't matter much how we approximate the timestamp, as long as we\n> do so consistently (minimizing jitter would however be important).\n> \n> \n> Pardon the silly question, but what is the difference between \"frame\n> timestamp\"\n> and \"sensor timestamp\" in this context?  \n\nI'm curious here too (and I'm scared it's originated with some work I\ndid several weeks ago), do we have a specific distinction between these\nterms? or is it simply that the intel algorithm library referenced a\nframe timestamp (statParams->frame_timestamp), and that name has\npropagated ?\n\n\n--\nKieran\n\n\n\n> > >> - We should move away from IPU3Event, now that the IPA interface is\n> > >>    specific to each pipeline handler. Instead of a single\n> processEvent()\n> > >>    method in IPAIPU3Interface, we should have 3 methods, with\n> ad-hoc\n> > >>    arguments.\n> > >>\n> > >>> Signed-off-by: Umang Jain >\n> > >> Reviewed-by: Laurent Pinchart\n> >\n> > >>\n> > >>> ---\n> > >>>   include/libcamera/ipa/ipu3.mojom     | 1 +\n> > >>>   src/ipa/ipu3/ipu3.cpp                | 4 +++-\n> > >>>   src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > >>>   3 files changed, 7 insertions(+), 3 deletions(-)\n> > >>>\n> > >>> diff --git a/include/libcamera/ipa/ipu3.mojom\n> b/include/libcamera/ipa/ipu3.mojom\n> > >>> index 32c046ad..29b4c805 100644\n> > >>> --- a/include/libcamera/ipa/ipu3.mojom\n> > >>> +++ b/include/libcamera/ipa/ipu3.mojom\n> > >>> @@ -21,6 +21,7 @@ enum IPU3Operations {\n> > >>>   struct IPU3Event {\n> > >>>           IPU3Operations op;\n> > >>>           uint32 frame;\n> > >>> + int64 frameTimestamp;\n> > >>>           uint32 bufferId;\n> > >>>           libcamera.ControlList controls;\n> > >>>   };\n> > >>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > >>> index 769c24d3..581297be 100644\n> > >>> --- a/src/ipa/ipu3/ipu3.cpp\n> > >>> +++ b/src/ipa/ipu3/ipu3.cpp\n> > >>> @@ -53,6 +53,7 @@ private:\n> > >>>           void processControls(unsigned int frame, const\n> ControlList &controls);\n> > >>>           void fillParams(unsigned int frame, ipu3_uapi_params\n> *params);\n> > >>>           void parseStatistics(unsigned int frame,\n> > >>> +                      int64_t frameTimestamp,\n> > >>>                                const ipu3_uapi_stats_3a *stats);\n> > >>>\n> > >>>           void setControls(unsigned int frame);\n> > >>> @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event\n> &event)\n> > >>>                   const ipu3_uapi_stats_3a *stats =\n> > >>>                           reinterpret_cast *>(mem.data());\n> > >>>\n> > >>> -         parseStatistics(event.frame, stats);\n> > >>> +         parseStatistics(event.frame, event.frameTimestamp,\n> stats);\n> > >>>                   break;\n> > >>>           }\n> > >>>           case EventFillParams: {\n> > >>> @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int\n> frame, ipu3_uapi_params *params)\n> > >>>   }\n> > >>>\n> > >>>   void IPAIPU3::parseStatistics(unsigned int frame,\n> > >>> +                       [[maybe_unused]] int64_t frameTimestamp,\n> > >>>                                 [[maybe_unused]] const\n> ipu3_uapi_stats_3a *stats)\n> > >>>   {\n> > >>>           ControlList ctrls(controls::controls);\n> > >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> index 750880ed..58923bc7 100644\n> > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> @@ -1357,6 +1357,8 @@ void\n> IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > >>>           if (!info)\n> > >>>                   return;\n> > >>>\n> > >>> + Request *request = info->request;\n> > >>> +\n> > >>>           if (buffer->metadata().status ==\n> FrameMetadata::FrameCancelled) {\n> > >>>                   info->metadataProcessed = true;\n> > >>>\n> > >>> @@ -1364,8 +1366,6 @@ void\n> IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > >>>                    * tryComplete() will delete info if it\n> completes the IPU3Frame.\n> > >>>                    * In that event, we must have obtained the\n> Request before hand.\n> > >>>                    */\n> > >>> -         Request *request = info->request;\n> > >>> -\n> > >>>                   if (frameInfos_.tryComplete(info))\n> > >>>                           pipe_->completeRequest(request);\n> > >>>\n> > >>> @@ -1376,6 +1376,7 @@ void\n> IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> > >>>           ev.op = ipa::ipu3::EventStatReady;\n> > >>>           ev.frame = info->id;\n> > >>>           ev.bufferId = info->statBuffer->cookie();\n> > >>> + ev.frameTimestamp =\n> request->metadata().get(controls::SensorTimestamp);\n> > >>>           ipa_->processEvent(ev);\n> > >>>   }\n> > >>>\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\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 C2676C3205\n\tfor ;\n\tSat, 29 May 2021 23:18:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6B4468922;\n\tSun, 30 May 2021 01:18:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1C4A602AA\n\tfor ;\n\tSun, 30 May 2021 01:18:29 +0200 (CEST)","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 014F550E;\n\tSun, 30 May 2021 01:18:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"M8RWEgb0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622330309;\n\tbh=tj3DXlKToWGuUgSezHExvQlLxDWmIdJuiHBQGTSX68Q=;\n\th=Reply-To:To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=M8RWEgb0Z9zNhlQO5mv4YzU7/MeO1/eNltXxH3kjieTREf3jR2sLaToR/OGLTPs+C\n\tYPH3LUnOWc5H0pcVPzVNYBTN3tEG86XjqEQTFIVK72C4yE6CTyDroWUekikE/i8aLG\n\t5DwAgN6tddhNhaGKc2I4r7E9lulOxAtoywFph59w=","To":"Naushir Patuck ,\n\tLaurent Pinchart ","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>\n\t\n\t<20210526200110.jhl7i47aefn56q4a@uno.localdomain>\n\t<989dd5bc-76ae-549c-9df8-719838561c2d@ideasonboard.com>\n\t\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Sun, 30 May 2021 00:18:26 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.8.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17347,"web_url":"https://patchwork.libcamera.org/comment/17347/","msgid":"","date":"2021-05-29T23:34:42","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Sun, May 30, 2021 at 12:18:26AM +0100, Kieran Bingham wrote:\n> On 28/05/2021 08:55, Naushir Patuck wrote:\n> > On Thu, 27 May 2021 at 17:10, Laurent Pinchart wrote:\n> >> On Thu, May 27, 2021 at 09:28:02AM +0530, Umang Jain wrote:\n> >>> On 5/27/21 1:31 AM, Jacopo Mondi wrote:\n> >>>> On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote:\n> >>>>> On Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:\n> >>>>>> Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via\n> >>>>>> IPU3Event. Frame timestamps are helpful to IPA algorithms to\n> >>>>>> convergence, by setting them via IPA stats.\n> >>>>>\n> >>>>> This looks good. There's room for improvement, but on top, as it would\n> >>>>>\n> >>>>> be too much yak-shaving:\n> >>>>>\n> >>>>> - Using std::chrono types would be nice for timestamps, but that should\n> >>>>> be done throughout libcamera.\n> >>>>\n> >>>> Just to mention that Nuash's utils::Duration will be a nice fit to\n> >>>> replace all the custom timestampings in the library\n> >>>>\n> >>>> For the patch:\n> >>>> - We create frameTimestamp by copying SensorTimestamp. Would it be\n> >>>> better to use the same name ?\n> >>>>\n> >>> The IPA is expecting a frame timestamp on it's side. It's entirely upto\n> >>> the PH to pass in, whatever it thinks closest as the timestamp of the\n> >>> frame. In this case, that's SensorTimestamp. I don't support that the\n> >>> naming should be changed just because we are passing in Sensor's\n> >>> timestamp. I am open to suggestions nevertheless, maybe an explicit\n> >>> comment on PH side :\n> >>>\n> >>> /* As of now, SensorTimestamp is the best approximation for the\n> >>> frame-timestamp */\n> >>>\n> >>> Interestingly raspberry-pi IPA also does similar,\n> >>>\n> >>> src/ipa/raspberrypi/raspberrypi.cpp:911: int64_t frameTimestamp =\n> >>> data.controls.get(controls::SensorTimestamp);\n> >>\n> >> The RPi IPA uses the timestamp to implement rate-limiting of the\n> >> algorithms. I'd expect IPAs to care more about the timestamp delta\n> >> between frames than the absolute timestamp, so from that point of view\n> >> it doesn't matter much how we approximate the timestamp, as long as we\n> >> do so consistently (minimizing jitter would however be important).\n> > \n> > Pardon the silly question, but what is the difference between \"frame\n> > timestamp\" and \"sensor timestamp\" in this context? \n> \n> I'm curious here too (and I'm scared it's originated with some work I\n> did several weeks ago), do we have a specific distinction between these\n> terms? or is it simply that the intel algorithm library referenced a\n> frame timestamp (statParams->frame_timestamp), and that name has\n> propagated ?\n\nThere's no strick definition, \"sensor timestamp\" is often used to refer\nto the start of exposure timestamp, while \"frame timestamp\" is often\nused to refer to the capture DMA timestamp (but is also used to refer to\nthe start of exposure timestamp). The latter isn't very useful as such,\nbut is often used as an approximation of the former. The distinction\nbetween the two terms doesn't matter much here, I don't think we need to\ncare.\n\n> >>>>> - We should move away from IPU3Event, now that the IPA interface is\n> >>>>> specific to each pipeline handler. Instead of a single processEvent()\n> >>>>> method in IPAIPU3Interface, we should have 3 methods, with ad-hoc\n> >>>>> arguments.\n> >>>>>\n> >>>>>> Signed-off-by: Umang Jain \n> >>>>>\n> >>>>> Reviewed-by: Laurent Pinchart \n> >>>>>\n> >>>>>> ---\n> >>>>>> include/libcamera/ipa/ipu3.mojom | 1 +\n> >>>>>> src/ipa/ipu3/ipu3.cpp | 4 +++-\n> >>>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> >>>>>> 3 files changed, 7 insertions(+), 3 deletions(-)\n> >>>>>>\n> >>>>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> >>>>>> index 32c046ad..29b4c805 100644\n> >>>>>> --- a/include/libcamera/ipa/ipu3.mojom\n> >>>>>> +++ b/include/libcamera/ipa/ipu3.mojom\n> >>>>>> @@ -21,6 +21,7 @@ enum IPU3Operations {\n> >>>>>> struct IPU3Event {\n> >>>>>> IPU3Operations op;\n> >>>>>> uint32 frame;\n> >>>>>> + int64 frameTimestamp;\n> >>>>>> uint32 bufferId;\n> >>>>>> libcamera.ControlList controls;\n> >>>>>> };\n> >>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> >>>>>> index 769c24d3..581297be 100644\n> >>>>>> --- a/src/ipa/ipu3/ipu3.cpp\n> >>>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n> >>>>>> @@ -53,6 +53,7 @@ private:\n> >>>>>> void processControls(unsigned int frame, const ControlList &controls);\n> >>>>>> void fillParams(unsigned int frame, ipu3_uapi_params *params);\n> >>>>>> void parseStatistics(unsigned int frame,\n> >>>>>> + int64_t frameTimestamp,\n> >>>>>> const ipu3_uapi_stats_3a *stats);\n> >>>>>>\n> >>>>>> void setControls(unsigned int frame);\n> >>>>>> @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)\n> >>>>>> const ipu3_uapi_stats_3a *stats =\n> >>>>>> reinterpret_cast(mem.data());\n> >>>>>>\n> >>>>>> - parseStatistics(event.frame, stats);\n> >>>>>> + parseStatistics(event.frame, event.frameTimestamp, stats);\n> >>>>>> break;\n> >>>>>> }\n> >>>>>> case EventFillParams: {\n> >>>>>> @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)\n> >>>>>> }\n> >>>>>>\n> >>>>>> void IPAIPU3::parseStatistics(unsigned int frame,\n> >>>>>> + [[maybe_unused]] int64_t frameTimestamp,\n> >>>>>> [[maybe_unused]] const ipu3_uapi_stats_3a *stats)\n> >>>>>> {\n> >>>>>> ControlList ctrls(controls::controls);\n> >>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>>> index 750880ed..58923bc7 100644\n> >>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>>>>> @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> >>>>>> if (!info)\n> >>>>>> return;\n> >>>>>>\n> >>>>>> + Request *request = info->request;\n> >>>>>> +\n> >>>>>> if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> >>>>>> info->metadataProcessed = true;\n> >>>>>>\n> >>>>>> @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> >>>>>> * tryComplete() will delete info if it completes the IPU3Frame.\n> >>>>>> * In that event, we must have obtained the Request before hand.\n> >>>>>> */\n> >>>>>> - Request *request = info->request;\n> >>>>>> -\n> >>>>>> if (frameInfos_.tryComplete(info))\n> >>>>>> pipe_->completeRequest(request);\n> >>>>>>\n> >>>>>> @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)\n> >>>>>> ev.op = ipa::ipu3::EventStatReady;\n> >>>>>> ev.frame = info->id;\n> >>>>>> ev.bufferId = info->statBuffer->cookie();\n> >>>>>> + ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);\n> >>>>>> ipa_->processEvent(ev);\n> >>>>>> }\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 D2C19C3206\n\tfor ;\n\tSat, 29 May 2021 23:34:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3547668922;\n\tSun, 30 May 2021 01:34:54 +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 42F53602AA\n\tfor ;\n\tSun, 30 May 2021 01:34:52 +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 79C8650E;\n\tSun, 30 May 2021 01:34:51 +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=\"nbDgKWUT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622331291;\n\tbh=+2STKHVPzyrBxaWhJr7w2ljHet2uAy+JMNjWTFcxnfg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nbDgKWUTGLmAQpHSxI6tG3Bs6uKnG+pEiORWkE+GhpOO6HBBMISLduWy8BjgpWKLW\n\tjosMR6nSO0V/7IEQ9jhORukCBEh0HqIvZk533n/x5jT2/MAilcbdkbcRUu7blmcDTG\n\txb0fCfpq2iDBmiRyHlH4sgdLn9pwy9N7I8tEN5qE=","Date":"Sun, 30 May 2021 02:34:42 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210526131025.675024-1-umang.jain@ideasonboard.com>\n\t<20210526131025.675024-3-umang.jain@ideasonboard.com>\n\t\n\t<20210526200110.jhl7i47aefn56q4a@uno.localdomain>\n\t<989dd5bc-76ae-549c-9df8-719838561c2d@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame\n\ttimestamps through IPU3Event","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]