[{"id":16920,"web_url":"https://patchwork.libcamera.org/comment/16920/","msgid":"","date":"2021-05-12T15:51:38","subject":"Re: [libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Store\n\ttimestamp in the correct Request metadata","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for this patch!\n\nOn Wed, 12 May 2021 at 09:47, Naushir Patuck wrote:\n>\n> Write the controls::SensorTimestamp value in the Request metadata when\n> the request is popped from the queue ready to run the pipeline. This\n> ensures that the timestamp is written to the correct Request item,\n> which may not be at the top of the queue when the Unicam buffer dequeue\n> occurs.\n>\n> Fixes: fcfb1dc02a6b (\"libcamera: raspberry: Report sensor timestamp\")\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 29 +++++++++++--------\n> 1 file changed, 17 insertions(+), 12 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 6fbdba0487bf..eb6d31670567 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -221,6 +221,8 @@ public:\n>\n> private:\n> void checkRequestCompleted();\n> + void fillRequestMetadata(const ControlList &bufferControls,\n> + Request *request);\n> void tryRunPipeline();\n> bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer);\n>\n> @@ -1416,18 +1418,6 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> << \", timestamp: \" << buffer->metadata().timestamp;\n>\n> if (stream == &unicam_[Unicam::Image]) {\n> - /*\n> - * Record the sensor timestamp in the Request.\n> - *\n> - * \\todo Do not assume the request in the front of the queue\n> - * is the correct one\n> - */\n> - Request *request = requestQueue_.front();\n> - ASSERT(request);\n> -\n> - request->metadata().set(controls::SensorTimestamp,\n> - buffer->metadata().timestamp);\n> -\n\nIndeed, I've actually been seeing segmentation faults at this line,\nmaybe ~10% of the time when running with the ov5647. I've applied\nthese patches and set the system running in a loop - all seems to be\ngood now!\n\n> /*\n> * Lookup the sensor controls used for this frame sequence from\n> * DelayedControl and queue them along with the frame buffer.\n> @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> }\n> }\n>\n> +void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n> + Request *request)\n> +{\n> + request->metadata().set(controls::SensorTimestamp,\n> + bufferControls.get(controls::SensorTimestamp));\n> +}\n\nI did wonder why this is worth a whole new function, but I see the\nnext commit adds something more to it!\n\n> +\n> void RPiCameraData::tryRunPipeline()\n> {\n> FrameBuffer *embeddedBuffer;\n> @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline()\n> /* See if a new ScalerCrop value needs to be applied. */\n> applyScalerCrop(request->controls());\n>\n> + /*\n> + * Clear the request metadata and fill it with some initial non-IPA\n> + * related controls. We clear it first because the request metadata\n> + * may have been populated if we have dropped the previous frame.\n> + */\n> + request->metadata().clear();\n\nAh yes, thank you. That's been most annoying since the\nControlList::merge function has started spitting out warnings!\n\nReviewed-by: David Plowman \nTested-by: David Plowman \n\nThanks\nDavid\n\n> + fillRequestMetadata(bayerFrame.controls, request);\n> +\n> /*\n> * Process all the user controls by the IPA. Once this is complete, we\n> * queue the ISP output buffer listed in the request to start the HW\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B91E2C31E3\n\tfor ;\n\tWed, 12 May 2021 15:51:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C8DB68919;\n\tWed, 12 May 2021 17:51:51 +0200 (CEST)","from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com\n\t[IPv6:2a00:1450:4864:20::32b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F200A602B7\n\tfor ;\n\tWed, 12 May 2021 17:51:49 +0200 (CEST)","by mail-wm1-x32b.google.com with SMTP id\n\ty124-20020a1c32820000b029010c93864955so1653681wmy.5\n\tfor ;\n\tWed, 12 May 2021 08:51:49 -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=\"BrvK4bLd\"; 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=s+Af1SqPdYUkLSdyo1AxkGY3KkUWQgGHVswMbtzsTEo=;\n\tb=BrvK4bLdtg7xWVEEBqS98TPSxuI0XIr36EmT8Rf3UxKwB2cuN1aAdl10x11PkjqKjU\n\t0kQUSC3KBzCm9H7As/lve8C3gwf6RuhwXKfRbvuKEBF0/AUkrq4OmOptJTqFsLxcbiPO\n\tHHvTkR1+feTxTmZXLcKc+lkzZZJ0gbze5pvPx9OGA5ROECFR+o+Q0A9oWV1uLOeRcG5R\n\tR5dHBLa9dabfsstZXi4/LUQLTTidj9+UMWyFdEf761BniC4vGsywOX5Vw6TrURc81Eq4\n\tX45cN1w/zWnsA6CfAuY5au0SzvjOdUmR5IZ7Y+ev9nsLMw1sLZyQE4xXyBTEa83qX28n\n\tlzVw==","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=s+Af1SqPdYUkLSdyo1AxkGY3KkUWQgGHVswMbtzsTEo=;\n\tb=BjJVEquIh0yHWEAbB9EpO6MUaugmvjsZxdeVX4/LeSYWg6jmgxxaTKYMt0X2GRtZzu\n\t+YtMdzLt3ZohNyN25rD4WsacUS9frSu7DZqjOmF1ZIbZAZJ11MYA8c4WWkuDgD3Ki7+z\n\tTQLVW33xvpIMbZU4sO29ds2Tz8g140w4RjW5LREmHgYR8jW6XDWbveSugsc48SZOttCO\n\ttMulIGKnxPCjtsW0V6o2WbLwYcFEAnOI75H7eEtwgjYsoZkotF15q3nNNlMgot9BajK8\n\t42GeTNBaJXYJ6DfhHVXnHJ3uty2ZKpx5mOmcfQ7JQ5dQjEtlHNlyOLwfPbLRsgSMj7sE\n\thLlQ==","X-Gm-Message-State":"AOAM533eIY8c8gQF+CHxsbiQpvE32M5EywEsjyMKdNoyCtKHu5AZCgeO\n\tUOfxaYytioRjXM4VN1I4fkDw2w35Jlxijzre5ETXzA==","X-Google-Smtp-Source":"ABdhPJzv4zTIbX4cuTbrUgZkCU05wUN7jInQY4BvFeK93iK2GJ6UkNf3lDXj0y3SNAbQvusCbwlXm/e2cUP4yi+fb/k=","X-Received":"by 2002:a1c:1d50:: with SMTP id\n\td77mr12750192wmd.114.1620834709643; \n\tWed, 12 May 2021 08:51:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20210512084744.1499469-1-naush@raspberrypi.com>","In-Reply-To":"<20210512084744.1499469-1-naush@raspberrypi.com>","From":"David Plowman ","Date":"Wed, 12 May 2021 16:51:38 +0100","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Store\n\ttimestamp in the correct Request metadata","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16989,"web_url":"https://patchwork.libcamera.org/comment/16989/","msgid":"","date":"2021-05-17T09:58:19","subject":"Re: [libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Store\n\ttimestamp in the correct Request metadata","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi,\n\nGentle ping to get some feedback on this patch series. It fixes a possible\nsegfault that has been reported, and would be nice to get in soon.\n\nRegards,\nNaush\n\n\nOn Wed, 12 May 2021 at 09:47, Naushir Patuck wrote:\n\n> Write the controls::SensorTimestamp value in the Request metadata when\n> the request is popped from the queue ready to run the pipeline. This\n> ensures that the timestamp is written to the correct Request item,\n> which may not be at the top of the queue when the Unicam buffer dequeue\n> occurs.\n>\n> Fixes: fcfb1dc02a6b (\"libcamera: raspberry: Report sensor timestamp\")\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 29 +++++++++++--------\n> 1 file changed, 17 insertions(+), 12 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 6fbdba0487bf..eb6d31670567 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -221,6 +221,8 @@ public:\n>\n> private:\n> void checkRequestCompleted();\n> + void fillRequestMetadata(const ControlList &bufferControls,\n> + Request *request);\n> void tryRunPipeline();\n> bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer\n> *&embeddedBuffer);\n>\n> @@ -1416,18 +1418,6 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer\n> *buffer)\n> << \", timestamp: \" << buffer->metadata().timestamp;\n>\n> if (stream == &unicam_[Unicam::Image]) {\n> - /*\n> - * Record the sensor timestamp in the Request.\n> - *\n> - * \\todo Do not assume the request in the front of the\n> queue\n> - * is the correct one\n> - */\n> - Request *request = requestQueue_.front();\n> - ASSERT(request);\n> -\n> - request->metadata().set(controls::SensorTimestamp,\n> - buffer->metadata().timestamp);\n> -\n> /*\n> * Lookup the sensor controls used for this frame sequence\n> from\n> * DelayedControl and queue them along with the frame\n> buffer.\n> @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const\n> ControlList &controls)\n> }\n> }\n>\n> +void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n> + Request *request)\n> +{\n> + request->metadata().set(controls::SensorTimestamp,\n> +\n> bufferControls.get(controls::SensorTimestamp));\n> +}\n> +\n> void RPiCameraData::tryRunPipeline()\n> {\n> FrameBuffer *embeddedBuffer;\n> @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline()\n> /* See if a new ScalerCrop value needs to be applied. */\n> applyScalerCrop(request->controls());\n>\n> + /*\n> + * Clear the request metadata and fill it with some initial non-IPA\n> + * related controls. We clear it first because the request metadata\n> + * may have been populated if we have dropped the previous frame.\n> + */\n> + request->metadata().clear();\n> + fillRequestMetadata(bayerFrame.controls, request);\n> +\n> /*\n> * Process all the user controls by the IPA. Once this is\n> complete, we\n> * queue the ISP output buffer listed in the request to start the\n> HW\n> --\n> 2.25.1\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 1C225C31FC\n\tfor ;\n\tMon, 17 May 2021 09:58:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F8F368915;\n\tMon, 17 May 2021 11:58:38 +0200 (CEST)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9DB01602B2\n\tfor ;\n\tMon, 17 May 2021 11:58:36 +0200 (CEST)","by mail-lj1-x22d.google.com with SMTP id f12so6463180ljp.2\n\tfor ;\n\tMon, 17 May 2021 02:58:36 -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=\"gJSrlnXN\"; 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\tbh=jrj0KcT1wESoQ/Dp3WR8tsD+QXVxSw8lemaAsBwded4=;\n\tb=gJSrlnXNxX+dV3LxuX76EOQzhYTRxZEO0f5XVi3Wln9znB+SP9E0E3usHdgCQP8ZuN\n\tdKhx8SMNuTvIozcxVSTjCJJohtDBoNrySvq8mQFAdk82ABZg2bz6FJwuQKr8LXrZvmft\n\twAfzVmI7ZkFsrktgXOYS8mLVdrT4YkxE4xQSUQkPiIDvnQF/1497BHwyFmhmmHVOP5nN\n\ti4w7GYhvBqTNztaMiuILFoQLQhQZg63zQFVYVzuK20KmOTuKCJEcFGYYAgfe2rycOGV/\n\tJapgj3zs1hpX6IjLKJw5Yyn3cQjlBz2gJ0LE6l3w88GzZTBGSP9YfvBtYHOdozRnSLgp\n\tEs8A==","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;\n\tbh=jrj0KcT1wESoQ/Dp3WR8tsD+QXVxSw8lemaAsBwded4=;\n\tb=ceiHzltjClhlHi3ANMeTEixkAekXLVrmh5OIho0PVLphoAqHndn9PIX2beZwS+IEug\n\tBUDwkMYdwQmUR+jFe+iT5be0RKeBn0Ohh04+r88vNatE5Z3hHGPlb6ZbHUWPGQflrnlf\n\tKjnHAgIEgZVPKuhi+z+5/RrQuq1k3g+zqOulzdq/DJrNrehOoeLEk2+3JbVOaMjavqWt\n\tdH3CMvNn0vX1/KxXEqxIFYJmmYjKwxzLJK5KhXkVGIrFvG0IVlmMo+qi0ZxeJfIRRCVn\n\tAcsSGAE2l9yGuBbwzVqnjjKpQqlSnB6TX1Kqpgx8wbz2vRUk/OSRYhwQhnhgudXSnkXA\n\taV6g==","X-Gm-Message-State":"AOAM530IqjsOMTv++vUYlRHL6UbLPOMDL4ZYb54pG2FRe2PQm+u+YwH7\n\tTyFZZy5N8sTjotr8gpmyj3CxUB9rqddabzvP7lZWbB3rRJs=","X-Google-Smtp-Source":"ABdhPJzJzwciUT8UISeZsYKAEZjF0xwuTk8hGVPNt7Ji16Ypd7vZqlOW+oT2j4IvWaMU9xxawkwN4QnRNJC61UYUTBs=","X-Received":"by 2002:a2e:9b58:: with SMTP id\n\to24mr17987900ljj.253.1621245515257; \n\tMon, 17 May 2021 02:58:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20210512084744.1499469-1-naush@raspberrypi.com>","In-Reply-To":"<20210512084744.1499469-1-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Mon, 17 May 2021 10:58:19 +0100","Message-ID":"","To":"libcamera devel ","Content-Type":"multipart/alternative; boundary=\"00000000000007e92005c283a355\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Store\n\ttimestamp in the correct Request metadata","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\" "}},{"id":16991,"web_url":"https://patchwork.libcamera.org/comment/16991/","msgid":"<8068cac9-7e4e-5c1d-50b5-bb21535fd565@ideasonboard.com>","date":"2021-05-17T12:40:27","subject":"Re: [libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Store\n\ttimestamp in the correct Request metadata","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 12/05/2021 09:47, Naushir Patuck wrote:\n> Write the controls::SensorTimestamp value in the Request metadata when\n> the request is popped from the queue ready to run the pipeline. This\n> ensures that the timestamp is written to the correct Request item,\n> which may not be at the top of the queue when the Unicam buffer dequeue\n> occurs.\n> \n> Fixes: fcfb1dc02a6b (\"libcamera: raspberry: Report sensor timestamp\")\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 29 +++++++++++--------\n> 1 file changed, 17 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 6fbdba0487bf..eb6d31670567 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -221,6 +221,8 @@ public:\n> \n> private:\n> \tvoid checkRequestCompleted();\n> +\tvoid fillRequestMetadata(const ControlList &bufferControls,\n> +\t\t\t\t Request *request);\n> \tvoid tryRunPipeline();\n> \tbool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer);\n> \n> @@ -1416,18 +1418,6 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> \n> \tif (stream == &unicam_[Unicam::Image]) {\n> -\t\t/*\n> -\t\t * Record the sensor timestamp in the Request.\n> -\t\t *\n> -\t\t * \\todo Do not assume the request in the front of the queue\n> -\t\t * is the correct one\n> -\t\t */\n> -\t\tRequest *request = requestQueue_.front();\n> -\t\tASSERT(request);\n> -\n> -\t\trequest->metadata().set(controls::SensorTimestamp,\n> -\t\t\t\t\tbuffer->metadata().timestamp);\n> -\n> \t\t/*\n> \t\t * Lookup the sensor controls used for this frame sequence from\n> \t\t * DelayedControl and queue them along with the frame buffer.\n> @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)\n> \t}\n> }\n> \n> +void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,\n> +\t\t\t\t\tRequest *request)\n> +{\n> +\trequest->metadata().set(controls::SensorTimestamp,\n> +\t\t\t\tbufferControls.get(controls::SensorTimestamp));\n> +}\n> +\n> void RPiCameraData::tryRunPipeline()\n> {\n> \tFrameBuffer *embeddedBuffer;\n> @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline()\n> \t/* See if a new ScalerCrop value needs to be applied. */\n> \tapplyScalerCrop(request->controls());\n> \n> +\t/*\n> +\t * Clear the request metadata and fill it with some initial non-IPA\n> +\t * related controls. We clear it first because the request metadata\n> +\t * may have been populated if we have dropped the previous frame.\n> +\t */\n\nAha, I was going to say is this the right place to clear this, and then\nI re-read that comment and now I understand why it's here.\n\nSo I think this is fine.\n\nIs there anything else that would have to be cleared down if a request\ngets 're-used' internally?\n\nI suspect not at the moment, but it may be that we need a specific call\non a request to clean it up more generically.\n\nThis should be fine though\n\nReviewed-by: Kieran Bingham \n\n\n\nHrm, thinking about it more, i think this comes down to the\nControlList->merge() doesn't it ... if the entry is already there it\nwon't be updated? I almost feel like in this case - it should be\nupdated, because it's newer and more correct information...\n\n\n\n> +\trequest->metadata().clear();\n> +\tfillRequestMetadata(bayerFrame.controls, request);\n> +\n> \t/*\n> \t * Process all the user controls by the IPA. Once this is complete, we\n> \t * queue the ISP output buffer listed in the request to start the HW\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 6B9E4C31FC\n\tfor ;\n\tMon, 17 May 2021 12:40:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B718168918;\n\tMon, 17 May 2021 14:40:31 +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 09FDF602C6\n\tfor ;\n\tMon, 17 May 2021 14:40:31 +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 63C5788F;\n\tMon, 17 May 2021 14:40:30 +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=\"vLt5dK5x\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1621255230;\n\tbh=VKGCYRzi+NSOfn3xlhPqpYNXheMAH/OXYp1Eb3u3MRM=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=vLt5dK5x7f/ABpHaRI7h2ofeIEJyycxcWavyH+pKGIcRR1iz7fbCwJSEnhPjn5BHN\n\tpkyGzUgRN4j0zB5gg6WxNjCm/CaJHHYsmBmfW9E/JCa3CYW6EUiBc5IePcF9e27b21\n\t9b+qwV5sQcetybnPzN78GLVxMRqi3RHfBrqzxWnE=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210512084744.1499469-1-naush@raspberrypi.com>","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"<8068cac9-7e4e-5c1d-50b5-bb21535fd565@ideasonboard.com>","Date":"Mon, 17 May 2021 13:40:27 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210512084744.1499469-1-naush@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Store\n\ttimestamp in the correct Request metadata","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16994,"web_url":"https://patchwork.libcamera.org/comment/16994/","msgid":"","date":"2021-05-17T13:24:27","subject":"Re: [libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Store\n\ttimestamp in the correct Request metadata","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for your feedback.\n\nOn Mon, 17 May 2021 at 13:40, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On 12/05/2021 09:47, Naushir Patuck wrote:\n> > Write the controls::SensorTimestamp value in the Request metadata when\n> > the request is popped from the queue ready to run the pipeline. This\n> > ensures that the timestamp is written to the correct Request item,\n> > which may not be at the top of the queue when the Unicam buffer dequeue\n> > occurs.\n> >\n> > Fixes: fcfb1dc02a6b (\"libcamera: raspberry: Report sensor timestamp\")\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 29 +++++++++++--------\n> > 1 file changed, 17 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 6fbdba0487bf..eb6d31670567 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -221,6 +221,8 @@ public:\n> >\n> > private:\n> > void checkRequestCompleted();\n> > + void fillRequestMetadata(const ControlList &bufferControls,\n> > + Request *request);\n> > void tryRunPipeline();\n> > bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer\n> *&embeddedBuffer);\n> >\n> > @@ -1416,18 +1418,6 @@ void\n> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > << \", timestamp: \" << buffer->metadata().timestamp;\n> >\n> > if (stream == &unicam_[Unicam::Image]) {\n> > - /*\n> > - * Record the sensor timestamp in the Request.\n> > - *\n> > - * \\todo Do not assume the request in the front of the\n> queue\n> > - * is the correct one\n> > - */\n> > - Request *request = requestQueue_.front();\n> > - ASSERT(request);\n> > -\n> > - request->metadata().set(controls::SensorTimestamp,\n> > - buffer->metadata().timestamp);\n> > -\n> > /*\n> > * Lookup the sensor controls used for this frame sequence\n> from\n> > * DelayedControl and queue them along with the frame\n> buffer.\n> > @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const\n> ControlList &controls)\n> > }\n> > }\n> >\n> > +void RPiCameraData::fillRequestMetadata(const ControlList\n> &bufferControls,\n> > + Request *request)\n> > +{\n> > + request->metadata().set(controls::SensorTimestamp,\n> > +\n> bufferControls.get(controls::SensorTimestamp));\n> > +}\n> > +\n> > void RPiCameraData::tryRunPipeline()\n> > {\n> > FrameBuffer *embeddedBuffer;\n> > @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline()\n> > /* See if a new ScalerCrop value needs to be applied. */\n> > applyScalerCrop(request->controls());\n> >\n> > + /*\n> > + * Clear the request metadata and fill it with some initial non-IPA\n> > + * related controls. We clear it first because the request metadata\n> > + * may have been populated if we have dropped the previous frame.\n> > + */\n>\n> Aha, I was going to say is this the right place to clear this, and then\n> I re-read that comment and now I understand why it's here.\n>\n> So I think this is fine.\n>\n> Is there anything else that would have to be cleared down if a request\n> gets 're-used' internally?\n>\n\nI think that's it.... for now :-)\n\n\n>\n> I suspect not at the moment, but it may be that we need a specific call\n> on a request to clean it up more generically.\n>\n> This should be fine though\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n>\n> Hrm, thinking about it more, i think this comes down to the\n> ControlList->merge() doesn't it ... if the entry is already there it\n> won't be updated? I almost feel like in this case - it should be\n> updated, because it's newer and more correct information...\n>\n\nCorrect. It's a consequence of doing the ControlList::merge() now.\nI think ControlList::merge() follows the convention of std::map::merge()\nwhere it does not merge if duplicate keys exist between the two maps.\n\nRegards,\nNaush\n\n\n\n>\n>\n>\n> > + request->metadata().clear();\n> > + fillRequestMetadata(bayerFrame.controls, request);\n> > +\n> > /*\n> > * Process all the user controls by the IPA. Once this is\n> complete, we\n> > * queue the ISP output buffer listed in the request to start the\n> HW\n> >\n>\n> --\n> Regards\n> --\n> Kieran\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 7552DC31FB\n\tfor ;\n\tMon, 17 May 2021 13:24:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C11DF6891B;\n\tMon, 17 May 2021 15:24:46 +0200 (CEST)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6ECF3602C6\n\tfor ;\n\tMon, 17 May 2021 15:24:44 +0200 (CEST)","by mail-lj1-x235.google.com with SMTP id v6so7232553ljj.5\n\tfor ;\n\tMon, 17 May 2021 06:24:44 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"U9peVvSi\"; 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=X5eLQDGNXzND8FTzKOCskUY092gtQwmwKXtJt9NQcf8=;\n\tb=U9peVvSiGtjy3eKNi+FqXUj8gQbjhAzia6eRN+1JOXLH80FC9KpA8qkpletIWGovJF\n\thnXT5C7tAL3tzCmDtWVU/Vtu8YlyBdXbyPI2kHVa+kQ2QeUaAbspusl3K3e5NBoyJrfK\n\tzQsqw001pNPGbQIQA/nv+BTR3kPENhR3XjXvH6af6N7511rOju+vg3NcWyrWn82kFf5z\n\tD9s/gXftzQQmKkR9KzYVycNEjEHIXAhsOARibNvLs76bYWj02En5ZIo/jHkSFBWwhMJT\n\tNvojOK4CAhkE0G9I1fK9O8wOFzso37BnKxTMMcXZfKifOTFCQQuaBuGWn+gFaCB0CNwX\n\t83Rw==","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=X5eLQDGNXzND8FTzKOCskUY092gtQwmwKXtJt9NQcf8=;\n\tb=jvTbLVTEzQMnZd5NZ9FOWV1jOnRwEWly0hgSgFb0zLHdhUBJAhroRLXqpkZexilFlI\n\taTNt6TjoOdvXkkgP75kFzWEK1VFTw5sa5lNpZbmIwNmTsQmF/7ek92VDflzQ5rkA8Des\n\t4IB5/EOH/juU1o/ytGZdMwNDHzs7U0TM9GUX6BdsssEw3JyoZXTdaSV2q4wMm3kCG+CZ\n\t2aMltmPVJu7l4asPlZN6x4Et2rhRTfYpWJbWvF6rOPai0nDFJLLIoPqKUXrq3Hf2C4BJ\n\tKrqcbUznKBhuXZU3cK2nHJ87S9vzVgIB9eCr54r5r3jiLq6SWAxrjJt97H3uo7hj5tVp\n\tbfqQ==","X-Gm-Message-State":"AOAM533HObirdY7ple8T3Fo8XoU8QxcldRvlUO9927b/H92T3KfEqT1l\n\tW/HmWAoL0IeLyTcWL7XW+xfYwON03wmTWlNR/KaCsjX2VbU6gg==","X-Google-Smtp-Source":"ABdhPJyRTYsrjDWc6Jrqac8hh/EKsoZ6CumdDWI637y0qoUZ17X7ZqUSkFjLj3e/epeDQNv22PNW6sQ3R3RDAPUCjEg=","X-Received":"by 2002:a2e:9985:: with SMTP id\n\tw5mr29167386lji.169.1621257883636; \n\tMon, 17 May 2021 06:24:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20210512084744.1499469-1-naush@raspberrypi.com>\n\t<8068cac9-7e4e-5c1d-50b5-bb21535fd565@ideasonboard.com>","In-Reply-To":"<8068cac9-7e4e-5c1d-50b5-bb21535fd565@ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 17 May 2021 14:24:27 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"0000000000003e763505c286841a\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Store\n\ttimestamp in the correct Request metadata","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\" "}}]