[{"id":17602,"web_url":"https://patchwork.libcamera.org/comment/17602/","msgid":"","date":"2021-06-17T06:01:45","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: simple: Fix\n\tcrash when storing timestamp in metadata","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2021-06-17 02:44:22 +0300, Laurent Pinchart wrote:\n> Commit 922833f774f6 (\"libcamera: simple: Report sensor timestamp\")\n> unconditionally tries to access the request through the capture buffer\n> to store the capture timestamp in the metadata. This causes a null\n> pointer dereference when using a converter, as the capture buffers are\n> free-wheeling in that case, and not associated with a request.\n> \n> Fix this by getting the request from the user-facing buffer, which can\n> be the capture buffer when no converter is used.\n> \n> Fixes: 922833f774f6 (\"libcamera: simple: Report sensor timestamp\")\n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++---\n> 1 file changed, 17 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 36fd9b852b33..e63d0bfd2fcb 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -1127,14 +1127,28 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> \t}\n> \n> \t/*\n> -\t * Record the sensor's timestamp in the request metadata.\n> +\t * Record the sensor's timestamp in the request metadata. The request\n> +\t * needs to be obtained from the user-facing buffer, as internal\n> +\t * buffers are free-wheeling and have no request associated with them.\n> \t *\n> \t * \\todo The sensor timestamp should be better estimated by connecting\n> \t * to the V4L2Device::frameStart signal if the platform provides it.\n> \t */\n> \tRequest *request = buffer->request();\n> -\trequest->metadata().set(controls::SensorTimestamp,\n> -\t\t\t\tbuffer->metadata().timestamp);\n> +\n> +\tif (data->useConverter_ && !data->converterQueue_.empty()) {\n> +\t\tconst std::map &outputs =\n> +\t\t\tdata->converterQueue_.front();\n> +\t\tif (!outputs.empty()) {\n> +\t\t\tFrameBuffer *outputBuffer = outputs.begin()->second;\n> +\t\t\tif (outputBuffer)\n> +\t\t\t\trequest = outputBuffer->request();\n> +\t\t}\n> +\t}\n> +\n> +\tif (request)\n> +\t\trequest->metadata().set(controls::SensorTimestamp,\n> +\t\t\t\t\tbuffer->metadata().timestamp);\n> \n> \t/*\n> \t * Queue the captured and the request buffer to the converter if format\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 A7CCEBD78E\n\tfor ;\n\tThu, 17 Jun 2021 06:01:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7A1E68941;\n\tThu, 17 Jun 2021 08:01:48 +0200 (CEST)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C41A960296\n\tfor ;\n\tThu, 17 Jun 2021 08:01:47 +0200 (CEST)","by mail-lf1-x129.google.com with SMTP id j2so8395797lfg.9\n\tfor ;\n\tWed, 16 Jun 2021 23:01:47 -0700 (PDT)","from localhost (h-46-59-88-219.A463.priv.bahnhof.se.\n\t[46.59.88.219]) by smtp.gmail.com with ESMTPSA id\n\tz7sm527684lji.65.2021.06.16.23.01.46\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 16 Jun 2021 23:01:46 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"SzlZxQKb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=jg73vkKG7uucT4rZAubJaAJZOxxpaKbJspH0VuzP8MM=;\n\tb=SzlZxQKbHCL/0UtOI0hEEvjDOPQfYUSuJvIYUfbiO76B3/K3AjKKQt2WZ6k4cxUzRy\n\tBp4Ozb4iV0TDM5FWGbueWccf2lBg5BmrzalF+WN3jp29hDiNf+pSvblWwaHyjuIn2NQU\n\t5wdXCDi/5TjzAYoG7JMHlLsFx66kL6eYKKiEWTdiAKcWcnfJeJjdtfF8pJ6B42cZCoo1\n\t4lXFa7XdZ4uO3p5XOoaMO/iC0M61tGBk9mFvKQ2/UgtzMSkX4IF5XhbNAbILEmbwVps2\n\tKFokCV3tLTv/Ua2VCTQnRrrGYrCEjNFSx7OnD+xhbl+ja23LjnRnzSqWgOQTvlJK4h0C\n\tL6uQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=jg73vkKG7uucT4rZAubJaAJZOxxpaKbJspH0VuzP8MM=;\n\tb=HZPt8LjrIMJNOzL52ieYtz7cob3+n8ED/1rN2slve2bf2BrSpQglPHYNUt8tSYJ6YW\n\tSIIW7OuGDVpO+ZWmkkC7QuMUNy/4t0ifQHAHowKk/P70bwEyBCA/TwaigrGqr1DqQsf/\n\tfqp6kd+XjQ+RQlrLnzsgZW0ErhNZI0q0FSxVo7aQmC30TtBF4rQehTueNlEY09a4ZzwW\n\tLZEcYEyjjEd4KjZi0XQF9hv2rlZmIo5NelrHSmELYyXXP2kDz3UAxdYoCoQX/645zdPH\n\tCFpOrLKJsYHKY4cUZIqKg8LS6QJBV2EoHbj8WxrLyTHI5TK8F92PNf+ylD6MFj7As7A9\n\tggKQ==","X-Gm-Message-State":"AOAM531+02jfST/Ro85ETEaXTRngFCtjVn+n+yyyMCIrrrmJf4UuMpsI\n\tYecIQs3lC3K2+04Hrodf1R19nA==","X-Google-Smtp-Source":"ABdhPJw5d3FyWtfaHXAb9u22NATCJckrg5AxVA6UcDUXN9K1Z6w8xzG4UST214RX4AvpycvlC8e6Yg==","X-Received":"by 2002:ac2:5511:: with SMTP id\n\tj17mr2653195lfk.432.1623909707032; \n\tWed, 16 Jun 2021 23:01:47 -0700 (PDT)","Date":"Thu, 17 Jun 2021 08:01:45 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210616234422.24789-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210616234422.24789-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: simple: Fix\n\tcrash when storing timestamp in 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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17610,"web_url":"https://patchwork.libcamera.org/comment/17610/","msgid":"<20210617081021.kmq6saqdq6graweb@uno.localdomain>","date":"2021-06-17T08:10:21","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: simple: Fix\n\tcrash when storing timestamp in metadata","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Jun 17, 2021 at 02:44:22AM +0300, Laurent Pinchart wrote:\n> Commit 922833f774f6 (\"libcamera: simple: Report sensor timestamp\")\n> unconditionally tries to access the request through the capture buffer\n> to store the capture timestamp in the metadata. This causes a null\n> pointer dereference when using a converter, as the capture buffers are\n> free-wheeling in that case, and not associated with a request.\n>\n> Fix this by getting the request from the user-facing buffer, which can\n> be the capture buffer when no converter is used.\n>\n> Fixes: 922833f774f6 (\"libcamera: simple: Report sensor timestamp\")\n> Signed-off-by: Laurent Pinchart \n\nMy bad, my understanding of the converters in the simple pipeline is\nlimited. Thanks for fixing\n\n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++---\n> 1 file changed, 17 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 36fd9b852b33..e63d0bfd2fcb 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -1127,14 +1127,28 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> \t}\n>\n> \t/*\n> -\t * Record the sensor's timestamp in the request metadata.\n> +\t * Record the sensor's timestamp in the request metadata. The request\n> +\t * needs to be obtained from the user-facing buffer, as internal\n> +\t * buffers are free-wheeling and have no request associated with them.\n> \t *\n> \t * \\todo The sensor timestamp should be better estimated by connecting\n> \t * to the V4L2Device::frameStart signal if the platform provides it.\n> \t */\n> \tRequest *request = buffer->request();\n> -\trequest->metadata().set(controls::SensorTimestamp,\n> -\t\t\t\tbuffer->metadata().timestamp);\n> +\n> +\tif (data->useConverter_ && !data->converterQueue_.empty()) {\n> +\t\tconst std::map &outputs =\n> +\t\t\tdata->converterQueue_.front();\n> +\t\tif (!outputs.empty()) {\n\nCan outputs be empty if !data->converterQueue_.empty() ?\n\n> +\t\t\tFrameBuffer *outputBuffer = outputs.begin()->second;\n> +\t\t\tif (outputBuffer)\n> +\t\t\t\trequest = outputBuffer->request();\n> +\t\t}\n> +\t}\n> +\n> +\tif (request)\n> +\t\trequest->metadata().set(controls::SensorTimestamp,\n> +\t\t\t\t\tbuffer->metadata().timestamp);\n\nWe could end up without a request\nBefore 922833f774f6 the Request * was simply retreived with\n\n Request *request = buffer->request();\n\nIf we can now end up without a Request *, how can we complete it here\nbelow ?\n\nThanks\n j\n\n>\n> \t/*\n> \t * Queue the captured and the request buffer to the converter if format\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 DB0EFBD78E\n\tfor ;\n\tThu, 17 Jun 2021 08:09:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6927968944;\n\tThu, 17 Jun 2021 10:09:34 +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 2E05B60297\n\tfor ;\n\tThu, 17 Jun 2021 10:09:33 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 0885740018;\n\tThu, 17 Jun 2021 08:09:31 +0000 (UTC)"],"Date":"Thu, 17 Jun 2021 10:10:21 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210617081021.kmq6saqdq6graweb@uno.localdomain>","References":"<20210616234422.24789-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210616234422.24789-1-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: simple: Fix\n\tcrash when storing timestamp in 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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17613,"web_url":"https://patchwork.libcamera.org/comment/17613/","msgid":"","date":"2021-06-17T09:12:28","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: simple: Fix\n\tcrash when storing timestamp in metadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Jun 17, 2021 at 10:10:21AM +0200, Jacopo Mondi wrote:\n> On Thu, Jun 17, 2021 at 02:44:22AM +0300, Laurent Pinchart wrote:\n> > Commit 922833f774f6 (\"libcamera: simple: Report sensor timestamp\")\n> > unconditionally tries to access the request through the capture buffer\n> > to store the capture timestamp in the metadata. This causes a null\n> > pointer dereference when using a converter, as the capture buffers are\n> > free-wheeling in that case, and not associated with a request.\n> >\n> > Fix this by getting the request from the user-facing buffer, which can\n> > be the capture buffer when no converter is used.\n> >\n> > Fixes: 922833f774f6 (\"libcamera: simple: Report sensor timestamp\")\n> > Signed-off-by: Laurent Pinchart \n> \n> My bad, my understanding of the converters in the simple pipeline is\n> limited. Thanks for fixing\n\nNo worries. I missed it during review too :-)\n\n> > ---\n> > src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++---\n> > 1 file changed, 17 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 36fd9b852b33..e63d0bfd2fcb 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -1127,14 +1127,28 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> > \t}\n> >\n> > \t/*\n> > -\t * Record the sensor's timestamp in the request metadata.\n> > +\t * Record the sensor's timestamp in the request metadata. The request\n> > +\t * needs to be obtained from the user-facing buffer, as internal\n> > +\t * buffers are free-wheeling and have no request associated with them.\n> > \t *\n> > \t * \\todo The sensor timestamp should be better estimated by connecting\n> > \t * to the V4L2Device::frameStart signal if the platform provides it.\n> > \t */\n> > \tRequest *request = buffer->request();\n> > -\trequest->metadata().set(controls::SensorTimestamp,\n> > -\t\t\t\tbuffer->metadata().timestamp);\n> > +\n> > +\tif (data->useConverter_ && !data->converterQueue_.empty()) {\n> > +\t\tconst std::map &outputs =\n> > +\t\t\tdata->converterQueue_.front();\n> > +\t\tif (!outputs.empty()) {\n> \n> Can outputs be empty if !data->converterQueue_.empty() ?\n\nThey shouldn't, but as there's the same sanity check in\nconverter->queueBuffers(), which is called *after* this code block, I\nthought it would be best to be cautious. Eventually we should probably\nrework this to only queue buffers for capture when there are request\nqueued, the same way te IPU3 pipeline handler does it, but that was a\ntoo big change for a fix.\n\n> > +\t\t\tFrameBuffer *outputBuffer = outputs.begin()->second;\n> > +\t\t\tif (outputBuffer)\n> > +\t\t\t\trequest = outputBuffer->request();\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (request)\n> > +\t\trequest->metadata().set(controls::SensorTimestamp,\n> > +\t\t\t\t\tbuffer->metadata().timestamp);\n> \n> We could end up without a request\n> Before 922833f774f6 the Request * was simply retreived with\n> \n> Request *request = buffer->request();\n> \n> If we can now end up without a Request *, how can we complete it here\n> below ?\n\nrequest can be null here if\n\n\tdata->useConverter && data->converterQueue_.empty()\n\nIn that case we'll take the `if (data->useConverter_)` branch, and won't\nreach the completeBuffer() call.\n\nIt's a little bit convoluted, the other option was to deplicate the\nrequest->metadata().set() call, which I didn't like much.\n\n> > \t/*\n> > \t * Queue the captured and the request buffer to the converter if format","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 B8367C3218\n\tfor ;\n\tThu, 17 Jun 2021 09:12:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3E4CF68944;\n\tThu, 17 Jun 2021 11:12:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB3F460297\n\tfor ;\n\tThu, 17 Jun 2021 11:12:50 +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 35974E7B;\n\tThu, 17 Jun 2021 11:12:50 +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=\"DmXaUnJE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623921170;\n\tbh=KXxzV4RmNHuFvjpfoApm/1OdgG7v6Yqu6HRYfUE1Nz8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DmXaUnJEckNcMmpFcU+Cp5hImNStOZBUauliroBCY4alPwMKE/taZizTRctxNlz2G\n\tqC4nC2AKM51k3Ao6mkkc0sjDRAhk6hLn1xU9K9oUACjCPdPSZt87GYVyxbVDF6qMHA\n\t3NvNkeDeC+FmT6WqATYUHIdBS20sfUEBgqmyZxrg=","Date":"Thu, 17 Jun 2021 12:12:28 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210616234422.24789-1-laurent.pinchart@ideasonboard.com>\n\t<20210617081021.kmq6saqdq6graweb@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210617081021.kmq6saqdq6graweb@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: simple: Fix\n\tcrash when storing timestamp in 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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17614,"web_url":"https://patchwork.libcamera.org/comment/17614/","msgid":"<20210617092731.e3dmyelqsxisaqix@uno.localdomain>","date":"2021-06-17T09:27:31","subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: simple: Fix\n\tcrash when storing timestamp in metadata","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Jun 17, 2021 at 12:12:28PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Thu, Jun 17, 2021 at 10:10:21AM +0200, Jacopo Mondi wrote:\n> > On Thu, Jun 17, 2021 at 02:44:22AM +0300, Laurent Pinchart wrote:\n> > > Commit 922833f774f6 (\"libcamera: simple: Report sensor timestamp\")\n> > > unconditionally tries to access the request through the capture buffer\n> > > to store the capture timestamp in the metadata. This causes a null\n> > > pointer dereference when using a converter, as the capture buffers are\n> > > free-wheeling in that case, and not associated with a request.\n> > >\n> > > Fix this by getting the request from the user-facing buffer, which can\n> > > be the capture buffer when no converter is used.\n> > >\n> > > Fixes: 922833f774f6 (\"libcamera: simple: Report sensor timestamp\")\n> > > Signed-off-by: Laurent Pinchart \n> >\n> > My bad, my understanding of the converters in the simple pipeline is\n> > limited. Thanks for fixing\n>\n> No worries. I missed it during review too :-)\n>\n> > > ---\n> > > src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++---\n> > > 1 file changed, 17 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > index 36fd9b852b33..e63d0bfd2fcb 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -1127,14 +1127,28 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> > > \t}\n> > >\n> > > \t/*\n> > > -\t * Record the sensor's timestamp in the request metadata.\n> > > +\t * Record the sensor's timestamp in the request metadata. The request\n> > > +\t * needs to be obtained from the user-facing buffer, as internal\n> > > +\t * buffers are free-wheeling and have no request associated with them.\n> > > \t *\n> > > \t * \\todo The sensor timestamp should be better estimated by connecting\n> > > \t * to the V4L2Device::frameStart signal if the platform provides it.\n> > > \t */\n> > > \tRequest *request = buffer->request();\n> > > -\trequest->metadata().set(controls::SensorTimestamp,\n> > > -\t\t\t\tbuffer->metadata().timestamp);\n> > > +\n> > > +\tif (data->useConverter_ && !data->converterQueue_.empty()) {\n> > > +\t\tconst std::map &outputs =\n> > > +\t\t\tdata->converterQueue_.front();\n> > > +\t\tif (!outputs.empty()) {\n> >\n> > Can outputs be empty if !data->converterQueue_.empty() ?\n>\n> They shouldn't, but as there's the same sanity check in\n> converter->queueBuffers(), which is called *after* this code block, I\n> thought it would be best to be cautious. Eventually we should probably\n> rework this to only queue buffers for capture when there are request\n> queued, the same way te IPU3 pipeline handler does it, but that was a\n> too big change for a fix.\n>\n> > > +\t\t\tFrameBuffer *outputBuffer = outputs.begin()->second;\n> > > +\t\t\tif (outputBuffer)\n> > > +\t\t\t\trequest = outputBuffer->request();\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tif (request)\n> > > +\t\trequest->metadata().set(controls::SensorTimestamp,\n> > > +\t\t\t\t\tbuffer->metadata().timestamp);\n> >\n> > We could end up without a request\n> > Before 922833f774f6 the Request * was simply retreived with\n> >\n> > Request *request = buffer->request();\n> >\n> > If we can now end up without a Request *, how can we complete it here\n> > below ?\n>\n> request can be null here if\n>\n> \tdata->useConverter && data->converterQueue_.empty()\n>\n> In that case we'll take the `if (data->useConverter_)` branch, and won't\n> reach the completeBuffer() call.\n\nOh right, I missed the condition check\n\n>\n> It's a little bit convoluted, the other option was to deplicate the\n> request->metadata().set() call, which I didn't like much.\n\nYes, not easy to follow in full, thanks for the clarifications\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> > > \t/*\n> > > \t * Queue the captured and the request buffer to the converter if format\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 26541C3218\n\tfor ;\n\tThu, 17 Jun 2021 09:26:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADCC76892E;\n\tThu, 17 Jun 2021 11:26:44 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 738D960297\n\tfor ;\n\tThu, 17 Jun 2021 11:26:43 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 3BCF6240010;\n\tThu, 17 Jun 2021 09:26:41 +0000 (UTC)"],"Date":"Thu, 17 Jun 2021 11:27:31 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210617092731.e3dmyelqsxisaqix@uno.localdomain>","References":"<20210616234422.24789-1-laurent.pinchart@ideasonboard.com>\n\t<20210617081021.kmq6saqdq6graweb@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: pipeline: simple: Fix\n\tcrash when storing timestamp in 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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]