[{"id":28494,"web_url":"https://patchwork.libcamera.org/comment/28494/","msgid":"<20240121040725.GB4378@pendragon.ideasonboard.com>","date":"2024-01-21T04:07:25","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Elias,\n\nThank you for the patch.\n\nOn Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote:\n> Vc4CameraData::findMatchBuffers may return successfully without setting\n> the embedded buffer. Make sure to initialize the buffer and id to avoid\n> accessing garbage data.\n\nHow so ? The function starts with\n\n\tif (bayerQueue_.empty())\n\t return false;\n\n\t/*\n\t * Find the embedded data buffer with a matching timestamp to pass to\n\t * the IPA. Any embedded buffers with a timestamp lower than the\n\t * current bayer buffer will be removed and re-queued to the driver.\n\t */\n\tuint64_t ts = bayerQueue_.front().buffer->metadata().timestamp;\n\tembeddedBuffer = nullptr;\n\nso I don't see how it can leave embeddedBuffer unset if it returns\nsuccessfully.\n\n> Without this change, libcamera v0.2.0 usually crashes with an assertion\n> error:\n> \n> ipa_base.cpp:397 assertion \"it != buffers_.end()\" failed in prepareIsp()\n\nI've seen this indeed, and will test your patch. Maybe it's due to the\nsecond change, where you set params.buffers.embedded to 0 ?\n\n> Signed-off-by: Elias Naur \n> ---\n> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> index 26102ea7..d76389f3 100644\n> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)\n> \n> void Vc4CameraData::tryRunPipeline()\n> {\n> -\tFrameBuffer *embeddedBuffer;\n> +\tFrameBuffer *embeddedBuffer = nullptr;\n> \tBayerFrame bayerFrame;\n> \n> \t/* If any of our request or buffer queues are empty, we cannot proceed. */\n> @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()\n> \tparams.requestControls = request->controls();\n> \tparams.ipaContext = request->sequence();\n> \tparams.delayContext = bayerFrame.delayContext;\n> +\tparams.buffers.embedded = 0;\n> \n> \tif (embeddedBuffer) {\n> \t\tunsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);","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 225DCC323E\n\tfor ;\n\tSun, 21 Jan 2024 04:07:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F2AF628B6;\n\tSun, 21 Jan 2024 05:07:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A7FC61D30\n\tfor ;\n\tSun, 21 Jan 2024 05:07:22 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D45B3BB2;\n\tSun, 21 Jan 2024 05:06:09 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705810044;\n\tbh=2fqZmDkeaob85dSkE7W+JD8sZi8auPq3amDz98a0NK4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=eKz+L7nwEoFy5VIYyw/+KVbOhRvpkR/dW9IhWAQALLxRKaCTFlWKWg7NoZDj2umee\n\tIc/wXDfcFwFpFqAWzwh0QsbRPNgeH62waM8zX9znYjvrtrkI7C+SVSry4CapltDyEf\n\tPpBy7ALyH5b9JKPavt/PKGRASFPPljNCNHcucpRG7lgt5KbLUazFNXePfsRdWxFdFQ\n\tO8JDUIyB0ROVJ92w9Tjp16QeNEtCg3dQK0NLTDDc3PpzbqcOlcPSyAFq6g/hmotW6W\n\tUpySZxlT1/mCz+spr6d33zm909QFXK6JgaAQ02jugHvOsCx2VFDWC11qcTY0A0HhWZ\n\t+ypNp9qwd/DzQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1705809970;\n\tbh=2fqZmDkeaob85dSkE7W+JD8sZi8auPq3amDz98a0NK4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PxjNAIayo2a5dI8sO8YPdVdqA466us365YDXqMVl9MMB1eSiXgNd4l9Nb0GNx11QZ\n\tIxHi49Ycqa6p0bqiJ0FNyFt1+7f1kUT4fOmWzrvUBXebGq7QED5gfGaZhxOz8Z5RcA\n\tFzLlYjsC3mqa9eVO8sHsXWA5AZknMi9+NsSzooHc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"PxjNAIay\"; dkim-atps=neutral","Date":"Sun, 21 Jan 2024 06:07:25 +0200","To":"Elias Naur ","Message-ID":"<20240121040725.GB4378@pendragon.ideasonboard.com>","References":"<20240120211508.14742-1-mail@eliasnaur.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240120211508.14742-1-mail@eliasnaur.com>","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28495,"web_url":"https://patchwork.libcamera.org/comment/28495/","msgid":"","date":"2024-01-21T13:03:49","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","submitter":{"id":159,"url":"https://patchwork.libcamera.org/api/people/159/","name":"Elias Naur","email":"mail@eliasnaur.com"},"content":"On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart\n wrote:\n>\n> Hi Elias,\n>\n> Thank you for the patch.\n>\n> On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote:\n> > Vc4CameraData::findMatchBuffers may return successfully without setting\n> > the embedded buffer. Make sure to initialize the buffer and id to avoid\n> > accessing garbage data.\n>\n> How so ? The function starts with\n>\n> if (bayerQueue_.empty())\n> return false;\n>\n> /*\n> * Find the embedded data buffer with a matching timestamp to pass to\n> * the IPA. Any embedded buffers with a timestamp lower than the\n> * current bayer buffer will be removed and re-queued to the driver.\n> */\n> uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp;\n> embeddedBuffer = nullptr;\n>\n> so I don't see how it can leave embeddedBuffer unset if it returns\n> successfully.\n>\n> > Without this change, libcamera v0.2.0 usually crashes with an assertion\n> > error:\n> >\n> > ipa_base.cpp:397 assertion \"it != buffers_.end()\" failed in prepareIsp()\n>\n> I've seen this indeed, and will test your patch. Maybe it's due to the\n> second change, where you set params.buffers.embedded to 0 ?\n>\n\nCorrect: the fix is the second change; the first change is defensive in case\nfindMatchBuffers for some reason doesn't set embeddedBuffer in case none\nis found.\n\n> > Signed-off-by: Elias Naur \n> > ---\n> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-\n> > 1 file changed, 2 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > index 26102ea7..d76389f3 100644\n> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)\n> >\n> > void Vc4CameraData::tryRunPipeline()\n> > {\n> > - FrameBuffer *embeddedBuffer;\n> > + FrameBuffer *embeddedBuffer = nullptr;\n> > BayerFrame bayerFrame;\n> >\n> > /* If any of our request or buffer queues are empty, we cannot proceed. */\n> > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()\n> > params.requestControls = request->controls();\n> > params.ipaContext = request->sequence();\n> > params.delayContext = bayerFrame.delayContext;\n> > + params.buffers.embedded = 0;\n> >\n> > if (embeddedBuffer) {\n> > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\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 07E7EBDB1C\n\tfor ;\n\tSun, 21 Jan 2024 13:04:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 206556291F;\n\tSun, 21 Jan 2024 14:04:03 +0100 (CET)","from mail-il1-x134.google.com (mail-il1-x134.google.com\n\t[IPv6:2607:f8b0:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 245EE61D46\n\tfor ;\n\tSun, 21 Jan 2024 14:04:02 +0100 (CET)","by mail-il1-x134.google.com with SMTP id\n\te9e14a558f8ab-3608e1d27ceso9502275ab.1\n\tfor ;\n\tSun, 21 Jan 2024 05:04:02 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705842243;\n\tbh=miLEIXfsLfrXcEKtmRluNStMqY8bro2JdTTrEfMA98c=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=toCBXq/XycW/HoUZPDvxVG1ng7dXXlzgOpA0XJeXYGdkQOgBdBAsNaMG5quHIFkYa\n\tetaOivhQm5X4/mVCf4CT/3gImpFzsG6hPFjKVvD3216dzZ8K9VUxpCCIiLSwZdpdoP\n\tpjHfkVuKgvrLtjOD9AWypewli1DzRw59YuTKuBDJJc2eAY8wSKnVT+xj0LqBMShyEK\n\t2KsTeUVo3huZS8Ma/0RdUnP7yTCMP0Vlr4INM+UtRjR5yOkwH7U6NE+v8HSgsjxjfD\n\tFVYg3S76f7B2XJXhd/q2vW6yl/FmkwGYznqAILV+NFKI8hDgySDXEkvXqlJpqGEL2M\n\t3NTlAqAdmLGrw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=eliasnaur.com; s=google; t=1705842241; x=1706447041;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Pt0UCo7Cqux6xfHYxkE1w8ty6jXg+/eCDATYY4W0eFI=;\n\tb=qFZSNLZhkgvud+SU4EHAn8/AC5DR3z6xZnSAJzJFUWHPxuQjKDlSSbhpiTVNOVufq4\n\tnlknL8Z7r828MWqxSCDI4QLW9RMH2YWo2Im+qdbeE+jNZ/0usA1YX9wBZnHAhtZXe23B\n\t2NC3foK1VVOFSV88Z0e4hJQUfPX6EdJNRXq02tAeuyTDdaeOm2kAWUKGzswMWXIHl1mu\n\tEKb1+Aefp0rtZyeQOeApZ3X7AVNWhJu/guf5yeJb5fmCHv+kXmy9CwkYjWOlM6Fo4Zqw\n\trHMf2JUpV4tnmi7vXQhNs9DiXt6XQgTQ1tYJD+R7F0F6rXrpgtoq3cg7Y2TlQ/O3Y1Nz\n\t0GVQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=eliasnaur.com\n\theader.i=@eliasnaur.com\n\theader.b=\"qFZSNLZh\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1705842241; x=1706447041;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=Pt0UCo7Cqux6xfHYxkE1w8ty6jXg+/eCDATYY4W0eFI=;\n\tb=JTjdD3znLoAPh+1t8OSmKHIWLHiCpaGQtFF7LEpgpC5BgY+xond6ZvTH7CSvvL6C/k\n\tT+Dm2N4S/nbNpkzrW0NG0WCAic/xvON0AAzxVLOp87lA5fbsGXUyccsk9UlueOTseav4\n\tnuCM9Zluy8v/c5ljxNhI7qpcEpBcPq+opnyqrPjt4m0HX1zTfMyNsQlPUnJWHuRbpBYA\n\tNA2BtSllCz+mjF7HKah7rEQCpJY2BmIwkpKv4czb1x9EPwB1K4Yj8jw7K7j0hX/yC17v\n\tpzKjSgBeydjy2zYNV7Lt7yPD0AKW+I6b8W34WsXplzK5OJAm6iJgSGLLQUTfmw7RhYCu\n\tE+qg==","X-Gm-Message-State":"AOJu0YwpmYJOni8EBoiAC+4SlG0DLuUBmSyfSfFeF18iSxZVdzPy77xz\n\tn020l/7RLMFTskH+YBpjSCldjgoTFKCM1JPRLmU6Se3ehB8lPnjBe3EFzwrPww9lgGNkC//qC7x\n\t+DBeFFzv6DDnxe5KyBaKuLs38n6UbLmUNmJVc","X-Google-Smtp-Source":"AGHT+IGKVeNdAD5LR6kUFo5x5mAZzCCIXl9ClM8XLvqPfil0cPkWYgBWZK4Is+Ix2F3Ggoeqx3EAFgdVQx4xxWlFlCI=","X-Received":"by 2002:a92:d78b:0:b0:361:ae55:22b8 with SMTP id\n\td11-20020a92d78b000000b00361ae5522b8mr4372318iln.45.1705842240666;\n\tSun, 21 Jan 2024 05:04:00 -0800 (PST)","MIME-Version":"1.0","References":"<20240120211508.14742-1-mail@eliasnaur.com>\n\t<20240121040725.GB4378@pendragon.ideasonboard.com>","In-Reply-To":"<20240121040725.GB4378@pendragon.ideasonboard.com>","Date":"Sun, 21 Jan 2024 08:03:49 -0500","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","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","From":"Elias Naur via libcamera-devel ","Reply-To":"Elias Naur ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28502,"web_url":"https://patchwork.libcamera.org/comment/28502/","msgid":"<20240122073732.GF4378@pendragon.ideasonboard.com>","date":"2024-01-22T07:37:32","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Elias,\n\nOn Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote:\n> On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote:\n> > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote:\n> > > Vc4CameraData::findMatchBuffers may return successfully without setting\n> > > the embedded buffer. Make sure to initialize the buffer and id to avoid\n> > > accessing garbage data.\n> >\n> > How so ? The function starts with\n> >\n> > if (bayerQueue_.empty())\n> > return false;\n> >\n> > /*\n> > * Find the embedded data buffer with a matching timestamp to pass to\n> > * the IPA. Any embedded buffers with a timestamp lower than the\n> > * current bayer buffer will be removed and re-queued to the driver.\n> > */\n> > uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp;\n> > embeddedBuffer = nullptr;\n> >\n> > so I don't see how it can leave embeddedBuffer unset if it returns\n> > successfully.\n> >\n> > > Without this change, libcamera v0.2.0 usually crashes with an assertion\n> > > error:\n> > >\n> > > ipa_base.cpp:397 assertion \"it != buffers_.end()\" failed in prepareIsp()\n> >\n> > I've seen this indeed, and will test your patch. Maybe it's due to the\n> > second change, where you set params.buffers.embedded to 0 ?\n> \n> Correct: the fix is the second change;\n\nCould you please update the commit message to reflect this ? I read it\nas indicating that embeddedBuffer could be null, while the issue is that\nparams.buffers.embedded is not initialized.\n\n> the first change is defensive in case\n> findMatchBuffers for some reason doesn't set embeddedBuffer in case none\n> is found.\n\nI'm not sure I'd keep that change. I'll let Naush and David decide what\nthey like best.\n\n> > > Signed-off-by: Elias Naur \n> > > ---\n> > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-\n> > > 1 file changed, 2 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > index 26102ea7..d76389f3 100644\n> > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)\n> > >\n> > > void Vc4CameraData::tryRunPipeline()\n> > > {\n> > > - FrameBuffer *embeddedBuffer;\n> > > + FrameBuffer *embeddedBuffer = nullptr;\n> > > BayerFrame bayerFrame;\n> > >\n> > > /* If any of our request or buffer queues are empty, we cannot proceed. */\n> > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()\n> > > params.requestControls = request->controls();\n> > > params.ipaContext = request->sequence();\n> > > params.delayContext = bayerFrame.delayContext;\n> > > + params.buffers.embedded = 0;\n> > >\n> > > if (embeddedBuffer) {\n> > > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);","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 08D37C323E\n\tfor ;\n\tMon, 22 Jan 2024 07:37:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B12062931;\n\tMon, 22 Jan 2024 08:37:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87D1361D2D\n\tfor ;\n\tMon, 22 Jan 2024 08:37:28 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 77613221;\n\tMon, 22 Jan 2024 08:36:15 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705909050;\n\tbh=UIcjH3ZwfLxiQJNNNC6e/0c9R7dSp3RLHkgvs4gYDYs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=S362Ft2nE0vU5gdX6RaY+ueXXHNMpoptfczMrg14x4NBVf76AGLF9gpM/rTqccrK1\n\tsCoXX3yIbpXt7NV7KmCtmJPjObD32i5IiCOlY5RZ938e92Zg0gIpjggUtj5R5fv/wg\n\tmG2q+1m7yDtZ0vITy6E4d0NTviB2z0yz4o+gD2o9js2V+ND1F4u6MF7hCGQu3M3mNn\n\tCLuhFWzfYZtOzfar/uJDYbf/tdJ9zzTp8Bn6qBfM1fq+sK6HGkSHA9OQjmpO+YIGZk\n\tFAtJFWXoDycL2n66m13sTxGIoWeaWrNYhIHzLYZLwTJI3d0BxSKW9o1Me6xySYnf+n\n\tjPhmq71NXXAOw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1705908975;\n\tbh=UIcjH3ZwfLxiQJNNNC6e/0c9R7dSp3RLHkgvs4gYDYs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kCAE46OW6765erSYM49mkKIqAm8lb69ExMhTZi/zez+WHEHhVOJ+xrZf6omQFxHva\n\tHBV48BSQAZ7gcqHqYbUFhAYu2FsH+MmAOUvPkk0fUF/2yWh7+PI17Ngkf0sLNUe37q\n\tLlUWcl+A1JQH5PCbDb85IXAd8tviA2KWmYbwVRkU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"kCAE46OW\"; dkim-atps=neutral","Date":"Mon, 22 Jan 2024 09:37:32 +0200","To":"Elias Naur ","Message-ID":"<20240122073732.GF4378@pendragon.ideasonboard.com>","References":"<20240120211508.14742-1-mail@eliasnaur.com>\n\t<20240121040725.GB4378@pendragon.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] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28512,"web_url":"https://patchwork.libcamera.org/comment/28512/","msgid":"","date":"2024-01-22T10:18:44","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\n\nOn Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel <\nlibcamera-devel@lists.libcamera.org> wrote:\n>\n> Hi Elias,\n>\n> On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote:\n> > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote:\n> > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via\nlibcamera-devel wrote:\n> > > > Vc4CameraData::findMatchBuffers may return successfully without\nsetting\n> > > > the embedded buffer. Make sure to initialize the buffer and id to\navoid\n> > > > accessing garbage data.\n> > >\n> > > How so ? The function starts with\n> > >\n> > > if (bayerQueue_.empty())\n> > > return false;\n> > >\n> > > /*\n> > > * Find the embedded data buffer with a matching timestamp to\npass to\n> > > * the IPA. Any embedded buffers with a timestamp lower than\nthe\n> > > * current bayer buffer will be removed and re-queued to the\ndriver.\n> > > */\n> > > uint64_t ts =\nbayerQueue_.front().buffer->metadata().timestamp;\n> > > embeddedBuffer = nullptr;\n> > >\n> > > so I don't see how it can leave embeddedBuffer unset if it returns\n> > > successfully.\n> > >\n> > > > Without this change, libcamera v0.2.0 usually crashes with an\nassertion\n> > > > error:\n> > > >\n> > > > ipa_base.cpp:397 assertion \"it != buffers_.end()\" failed in\nprepareIsp()\n> > >\n> > > I've seen this indeed, and will test your patch. Maybe it's due to the\n> > > second change, where you set params.buffers.embedded to 0 ?\n> >\n> > Correct: the fix is the second change;\n>\n> Could you please update the commit message to reflect this ? I read it\n> as indicating that embeddedBuffer could be null, while the issue is that\n> params.buffers.embedded is not initialized.\n>\n> > the first change is defensive in case\n> > findMatchBuffers for some reason doesn't set embeddedBuffer in case none\n> > is found.\n>\n> I'm not sure I'd keep that change. I'll let Naush and David decide what\n> they like best.\n\nI agree, let's just keep the change to initialising\n params.buffers.embedded.\nWith this, the change itself looks reasonable so\n\nReviewed-by: Naushir Patuck \n\nHowever... I could have sworn the mojom auto generated class file for\nipa::RPi::PrepareParams would initialize all member variables to 0\n(including buffers.embedded) on default construction. I'm away right now\nso I cannot check this myself, but could anybody else check? I'm somewhat\nsurprised that this has not been reported more widely...\n\nThanks,\nNaush\n\n>\n>\n> > > > Signed-off-by: Elias Naur \n> > > > ---\n> > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-\n> > > > 1 file changed, 2 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\nb/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > index 26102ea7..d76389f3 100644\n> > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t\nmaxFrameLengthMs)\n> > > >\n> > > > void Vc4CameraData::tryRunPipeline()\n> > > > {\n> > > > - FrameBuffer *embeddedBuffer;\n> > > > + FrameBuffer *embeddedBuffer = nullptr;\n> > > > BayerFrame bayerFrame;\n> > > >\n> > > > /* If any of our request or buffer queues are empty, we\ncannot proceed. */\n> > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()\n> > > > params.requestControls = request->controls();\n> > > > params.ipaContext = request->sequence();\n> > > > params.delayContext = bayerFrame.delayContext;\n> > > > + params.buffers.embedded = 0;\n> > > >\n> > > > if (embeddedBuffer) {\n> > > > unsigned int embeddedId =\nunicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\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 4E5D9BDB1C\n\tfor ;\n\tMon, 22 Jan 2024 10:19:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90D596291F;\n\tMon, 22 Jan 2024 11:19:24 +0100 (CET)","from mail-yb1-xb34.google.com (mail-yb1-xb34.google.com\n\t[IPv6:2607:f8b0:4864:20::b34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6064628AD\n\tfor ;\n\tMon, 22 Jan 2024 11:19:21 +0100 (CET)","by mail-yb1-xb34.google.com with SMTP id\n\t3f1490d57ef6-da7ea62e76cso2170166276.3\n\tfor ;\n\tMon, 22 Jan 2024 02:19:21 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705918764;\n\tbh=u4EizIBYyzkgF7inZByPL/Z3TEocclP7wgcrwYJD0vE=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=eofrlIaoFX1/XqIYiBECN4ot2+mzk41hySsSVstMBAQX0l9snYPV0HLi0KXtq8Bje\n\tF9XDBxnJ4Sg+9ohGBvQ015CQQzp1oCtALezBmNO/Po8dtGho9BCtxIwBAPdZ8JICD9\n\tv1IpdbTxYHOSnarJ8PR8IX5eFPS4H4u+BD0CP8nZZJJVWmxtkudbuH8dZXN0hew3AD\n\t1TOw9L3DiHm0Ii5C4jlQGy5xSnUKqm4+5m/sQK41m7LGeLXCEebAnErAeaZMBLwC7g\n\t0yfg4JqKXNJaA82AQniHtOsc1cmGbVPKTe4AZNa0AmoduTFwCYK/xnoTaJbnumpMyT\n\tlnXMspZpfHu0Q==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1705918760; x=1706523560;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=v0SUxMt2HxwoHET+hK1ySODSB9hFB3b6R/HCyjm95qw=;\n\tb=nrUYXeOu+sNe4Rgj+NIbAYeX9tT8vC0EcxrnjDu2bUJEWSfexgFw/xEX768Yp51evP\n\t8BvhYC0/D1Jp2XaPf1OR3dcM160OstsDY2Tds8LcEt2P5lj608IdnYtBVl4NE6Mo69yt\n\taj666WETHBMjKjGzGThlawMVwltxns/d6+3kwAFg3oeQl/GDCDsXD67wCJj51pQT4K8X\n\tLyhJp4cNpSXtN99V6OgzU06CvrtuCMKGuQU+LOne91d98Gr5HCMXhtxRPhAT4GelzTlA\n\t9zZwpsYejNYEtv2j/jz9g0bqijhreqT0LYzenDnmicf7/cS/RN0C2HbDZheoc1iPGBZq\n\t55tg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"nrUYXeOu\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1705918760; x=1706523560;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=v0SUxMt2HxwoHET+hK1ySODSB9hFB3b6R/HCyjm95qw=;\n\tb=cPs10ePOCe1LHTjuXhI/q50sbN5F8LAOjfuhwNl84A3P8wOBZqH9KRlWsA6Xwy9gX9\n\tjzFROlm1oiUR4OYQzKs441TfRYC0xpgafSKwhfZDTkaYzTy9tOmG+E/uTH7PDDtVEZPd\n\tjEE4mvCNUy7BvU6Tu7csb51zBpCIpL1l5OmGGK4c9P5RSpIecV+scqueXTYniud73cnQ\n\tPng/ioA63HGgo3J5R2Eyay2IQzfKz/jA8+vriPjCGlE9DWoYAYpNf5qUhaHW/5WIXfwI\n\tG/l6+wZV0Bbw0QMVbwixWfhIjam1k/FJ2UoMKLaWXm+kQTaSDnrgA52ZMKYijmcMWBR/\n\tPF9g==","X-Gm-Message-State":"AOJu0YwO83JkNaRA5ZXwwWaXFopiK3BPONP9r6leXU/Dg0gXzbEL21+O\n\tGQnIjzrgsc4+sb/yXShcAOK8aBf7b+Bxwi32xGzjy2y/r+iNuS0FzFxTWxCLq/PuF5OXdsU6GYe\n\tRg2TG3i7VCEWaa95PJiTFfXEATbrDpTFcMYf79w==","X-Google-Smtp-Source":"AGHT+IHYVdlFrJ2PwY5hIdytjlenib0YZcJnmG8Z57fg8ziZohGxA4brN7GPGxmK+bedykAhdc3ipHsWiWYHxG3SIdI=","X-Received":"by 2002:a25:950:0:b0:dc2:53c8:8013 with SMTP id\n\tu16-20020a250950000000b00dc253c88013mr1902586ybm.44.1705918760519;\n\tMon, 22 Jan 2024 02:19:20 -0800 (PST)","MIME-Version":"1.0","References":"<20240120211508.14742-1-mail@eliasnaur.com>\n\t<20240121040725.GB4378@pendragon.ideasonboard.com>\n\t\n\t<20240122073732.GF4378@pendragon.ideasonboard.com>","In-Reply-To":"<20240122073732.GF4378@pendragon.ideasonboard.com>","Date":"Mon, 22 Jan 2024 10:18:44 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000bc9c10060f8629b6\"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org, Elias Naur ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28514,"web_url":"https://patchwork.libcamera.org/comment/28514/","msgid":"<20240122103107.GO4378@pendragon.ideasonboard.com>","date":"2024-01-22T10:31:07","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote:\n> On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel wrote:\n> > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote:\n> > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote:\n> > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote:\n> > > > > Vc4CameraData::findMatchBuffers may return successfully without setting\n> > > > > the embedded buffer. Make sure to initialize the buffer and id to avoid\n> > > > > accessing garbage data.\n> > > >\n> > > > How so ? The function starts with\n> > > >\n> > > >         if (bayerQueue_.empty())\n> > > >                 return false;\n> > > >\n> > > >         /*\n> > > >          * Find the embedded data buffer with a matching timestamp to pass to\n> > > >          * the IPA. Any embedded buffers with a timestamp lower than the\n> > > >          * current bayer buffer will be removed and re-queued to the driver.\n> > > >          */\n> > > >         uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp;\n> > > >         embeddedBuffer = nullptr;\n> > > >\n> > > > so I don't see how it can leave embeddedBuffer unset if it returns\n> > > > successfully.\n> > > >\n> > > > > Without this change, libcamera v0.2.0 usually crashes with an assertion\n> > > > > error:\n> > > > >\n> > > > >  ipa_base.cpp:397 assertion \"it != buffers_.end()\" failed in prepareIsp()\n> > > >\n> > > > I've seen this indeed, and will test your patch. Maybe it's due to the\n> > > > second change, where you set params.buffers.embedded to 0 ?\n> > >\n> > > Correct: the fix is the second change;\n> >\n> > Could you please update the commit message to reflect this ? I read it\n> > as indicating that embeddedBuffer could be null, while the issue is that\n> > params.buffers.embedded is not initialized.\n> >\n> > > the first change is defensive in case\n> > > findMatchBuffers for some reason doesn't set embeddedBuffer in case none\n> > > is found.\n> >\n> > I'm not sure I'd keep that change. I'll let Naush and David decide what\n> > they like best.\n> \n> I agree, let's just keep the change to initialising  params.buffers.embedded.\n> With this, the change itself looks reasonable so\n> \n> Reviewed-by: Naushir Patuck \n\nElias, would you be able to send a v2 ?\n\n> However... I could have sworn the mojom auto generated class file for\n> ipa::RPi::PrepareParams would initialize all member variables to 0 (including\n> buffers.embedded) on default construction.  I'm away right now so I cannot\n> check this myself, but could anybody else check?  I'm somewhat surprised that\n> this has not been reported more widely...\n\nI checked, and the constructor doesn't initialize all fields to 0. I'm\npretty surprised too to be honest.\n\n> > > > > Signed-off-by: Elias Naur \n> > > > > ---\n> > > > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-\n> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > index 26102ea7..d76389f3 100644\n> > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)\n> > > > >\n> > > > >  void Vc4CameraData::tryRunPipeline()\n> > > > >  {\n> > > > > -     FrameBuffer *embeddedBuffer;\n> > > > > +     FrameBuffer *embeddedBuffer = nullptr;\n> > > > >       BayerFrame bayerFrame;\n> > > > >\n> > > > >       /* If any of our request or buffer queues are empty, we cannot proceed. */\n> > > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()\n> > > > >       params.requestControls = request->controls();\n> > > > >       params.ipaContext = request->sequence();\n> > > > >       params.delayContext = bayerFrame.delayContext;\n> > > > > +     params.buffers.embedded = 0;\n> > > > >\n> > > > >       if (embeddedBuffer) {\n> > > > >               unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);","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 DE92DC323E\n\tfor ;\n\tMon, 22 Jan 2024 10:31:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3BA406291F;\n\tMon, 22 Jan 2024 11:31:05 +0100 (CET)","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 75589628AD\n\tfor ;\n\tMon, 22 Jan 2024 11:31:03 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 74C4D74A;\n\tMon, 22 Jan 2024 11:29:50 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1705919465;\n\tbh=HWKjXCdexErJz9RiL/UByYlao8mYLmjfgeLn0Rve++s=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=lCNiExvwpvVysLQsqcVktZTThgPpkxdUcGr+Fhyr5nODATSskPhLjmo0/Nht/Adpl\n\tYo4QVnLG7Q+7IJzklHY0C3Y5h+HKhRo0J+TvYNbfpdS6zCjSI075625IXRCvxJNKm+\n\tUwm/Jm0kWuebCKCbY5BtK1knYXg6QdOZxlK42AKv1TftkzhHVI0YH0MoTyI/2nQm0p\n\tcFKq3AA4veFZmwPL0gVazYr6+Axw8GpnI5TynDX51NZfgEDQztOdkZ8STD7aNPomgM\n\tcJzk4tJPP6ITxaoB3xfmXxHA6aW/p3YhG6qJZPgYJCISqQqVH3FpD73qpuQ+2RkIEc\n\tmaX4lSvyB/YWg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1705919390;\n\tbh=HWKjXCdexErJz9RiL/UByYlao8mYLmjfgeLn0Rve++s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ovDxHD7NkOAWuQ/BjKbFKJDF1fj+i3UYfXZO9sTz6grFYh8oo+taODATCT62kEIDY\n\tqdQcmWgpk6+CY6GFqDvH5ImqVGSwosma3tJe49PUV05q1uHh5H73fgnwsggHDW8bhV\n\toTIN0nzqdPJhkaQDnSaO9FFA60TQK9Q102ZkMKQY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ovDxHD7N\"; dkim-atps=neutral","Date":"Mon, 22 Jan 2024 12:31:07 +0200","To":"Naushir Patuck ","Message-ID":"<20240122103107.GO4378@pendragon.ideasonboard.com>","References":"<20240120211508.14742-1-mail@eliasnaur.com>\n\t<20240121040725.GB4378@pendragon.ideasonboard.com>\n\t\n\t<20240122073732.GF4378@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Elias Naur ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28533,"web_url":"https://patchwork.libcamera.org/comment/28533/","msgid":"","date":"2024-01-22T13:36:18","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","submitter":{"id":159,"url":"https://patchwork.libcamera.org/api/people/159/","name":"Elias Naur","email":"mail@eliasnaur.com"},"content":"On Mon, 22 Jan 2024 at 05:31, Laurent Pinchart\n wrote:\n>\n> On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote:\n> > On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel wrote:\n> > > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote:\n> > > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote:\n> > > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote:\n> > > > > > Vc4CameraData::findMatchBuffers may return successfully without setting\n> > > > > > the embedded buffer. Make sure to initialize the buffer and id to avoid\n> > > > > > accessing garbage data.\n> > > > >\n> > > > > How so ? The function starts with\n> > > > >\n> > > > > if (bayerQueue_.empty())\n> > > > > return false;\n> > > > >\n> > > > > /*\n> > > > > * Find the embedded data buffer with a matching timestamp to pass to\n> > > > > * the IPA. Any embedded buffers with a timestamp lower than the\n> > > > > * current bayer buffer will be removed and re-queued to the driver.\n> > > > > */\n> > > > > uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp;\n> > > > > embeddedBuffer = nullptr;\n> > > > >\n> > > > > so I don't see how it can leave embeddedBuffer unset if it returns\n> > > > > successfully.\n> > > > >\n> > > > > > Without this change, libcamera v0.2.0 usually crashes with an assertion\n> > > > > > error:\n> > > > > >\n> > > > > > ipa_base.cpp:397 assertion \"it != buffers_.end()\" failed in prepareIsp()\n> > > > >\n> > > > > I've seen this indeed, and will test your patch. Maybe it's due to the\n> > > > > second change, where you set params.buffers.embedded to 0 ?\n> > > >\n> > > > Correct: the fix is the second change;\n> > >\n> > > Could you please update the commit message to reflect this ? I read it\n> > > as indicating that embeddedBuffer could be null, while the issue is that\n> > > params.buffers.embedded is not initialized.\n> > >\n> > > > the first change is defensive in case\n> > > > findMatchBuffers for some reason doesn't set embeddedBuffer in case none\n> > > > is found.\n> > >\n> > > I'm not sure I'd keep that change. I'll let Naush and David decide what\n> > > they like best.\n> >\n\nI don't like uninitialized values in general, but I suppose fixing\nthose is better done\nby, say, compiler flags.\n\n> > I agree, let's just keep the change to initialising params.buffers.embedded.\n> > With this, the change itself looks reasonable so\n> >\n> > Reviewed-by: Naushir Patuck \n>\n> Elias, would you be able to send a v2 ?\n>\n\nDone.\n\nElias","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 9C008BDB1C\n\tfor ;\n\tMon, 22 Jan 2024 13:36:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A865162944;\n\tMon, 22 Jan 2024 14:36:32 +0100 (CET)","from mail-pg1-x532.google.com (mail-pg1-x532.google.com\n\t[IPv6:2607:f8b0:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0996E628B7\n\tfor ;\n\tMon, 22 Jan 2024 14:36:31 +0100 (CET)","by mail-pg1-x532.google.com with SMTP id\n\t41be03b00d2f7-53fa455cd94so1329190a12.2\n\tfor ;\n\tMon, 22 Jan 2024 05:36:30 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=eliasnaur.com header.i=@eliasnaur.com\n\theader.b=\"Fr2rpvE8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=eliasnaur.com; s=google; t=1705930589; x=1706535389;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=C7meogYXEZjER+yWe1+51r0nwH0TfUVDAdixvZXVemM=;\n\tb=Fr2rpvE8A8yk0jIQuEmCgQOSURnKn2+HVadcGY95UWMIlsf3NVtdFbfTjUdVRgb/UT\n\trFDIeFapGOYXblWCegL67BjrItcel6dlq8L10Rgax5s8aPpB99t6U3L5jKK5Z59xCW/3\n\t363BdxRq8en5PCUl4IAQq3QBaG25zk2Lw64xgIMeLc0avy31jBt/KfhKBch8ZpiGCJfe\n\t29iuo78+9I8hQ01xnpqoRKTkLDnZIs8lpC/2TNHTJDjSCghymU/wTBatw+TGAXStN9eZ\n\tAQs3lOQmZhLUocAfE8ZoN1rIcU8eC+F2nUG2kAsHXnKuIXWpeg288dK3k2RzhDFs666G\n\t/cIQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1705930589; x=1706535389;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=C7meogYXEZjER+yWe1+51r0nwH0TfUVDAdixvZXVemM=;\n\tb=hG2vOoBcm5KPzZ+sgzdFHPZuamVLoV5LsnUagNdaMAf4LUPGqmF5B4pw/byjcbCLi0\n\tpLiiJndfFNc5cV5LfQmABvw72yGezLhBUuoGnbHIv70l2hH330ojgwALaZQs0c5pIVF5\n\tS4PmUN7dYKkl0UenpXWmY3TTin4Chj3q7VG9nJZUiG0wc2mNph4n2RkdJjJzqJxKFvVB\n\tbXhZGe1tk4GK0/DapDV8MvKdcr3k657C/gy9bF1L8seHIYI8oMGJPUzaM+O2MxsiaSUo\n\tswBe8JuODo0yTgUSTCQOYSuMb4VnRc0abo0MdwMVtlvDjzUdEzS4poWpGWG6edGN8RpV\n\tKIKg==","X-Gm-Message-State":"AOJu0YzcYrAwHdfphM8nGtF8rJipYpqR+4pNNDHkMsnpz4u1icQqwuJh\n\tA2rn+6SRRTKgob5UDRnUZkDIKtvdBV6o7UIfjBwARylJZjE3o/l5Dfkr0VWMii29vvTsGqAG5GO\n\tPWUdsyZNJn5coMUlpx8VYUnoPpRvsPRMmivSrXr9hpVBWhvOPyw==","X-Google-Smtp-Source":"AGHT+IF0I9BDwhTWB4Sl4QudYMOyP5XEDMZ/1+LpgtTyPRK+yLWXnkaXRz4r+f+sx27AkYPRUnDZXIZNJ8d39DAMvX8=","X-Received":"by 2002:a17:90a:51c3:b0:28f:f2b5:3f23 with SMTP id\n\tu61-20020a17090a51c300b0028ff2b53f23mr1391005pjh.35.1705930589331;\n\tMon, 22 Jan 2024 05:36:29 -0800 (PST)","MIME-Version":"1.0","References":"<20240120211508.14742-1-mail@eliasnaur.com>\n\t<20240121040725.GB4378@pendragon.ideasonboard.com>\n\t\n\t<20240122073732.GF4378@pendragon.ideasonboard.com>\n\t\n\t<20240122103107.GO4378@pendragon.ideasonboard.com>","In-Reply-To":"<20240122103107.GO4378@pendragon.ideasonboard.com>","From":"Elias Naur ","Date":"Mon, 22 Jan 2024 08:36:18 -0500","Message-ID":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","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":28619,"web_url":"https://patchwork.libcamera.org/comment/28619/","msgid":"","date":"2024-01-24T09:17:11","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Mon, 22 Jan 2024 at 10:31, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote:\n> > On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel wrote:\n> > > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote:\n> > > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote:\n> > > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via\n> libcamera-devel wrote:\n> > > > > > Vc4CameraData::findMatchBuffers may return successfully without\n> setting\n> > > > > > the embedded buffer. Make sure to initialize the buffer and id\n> to avoid\n> > > > > > accessing garbage data.\n> > > > >\n> > > > > How so ? The function starts with\n> > > > >\n> > > > > if (bayerQueue_.empty())\n> > > > > return false;\n> > > > >\n> > > > > /*\n> > > > > * Find the embedded data buffer with a matching timestamp\n> to pass to\n> > > > > * the IPA. Any embedded buffers with a timestamp lower\n> than the\n> > > > > * current bayer buffer will be removed and re-queued to\n> the driver.\n> > > > > */\n> > > > > uint64_t ts =\n> bayerQueue_.front().buffer->metadata().timestamp;\n> > > > > embeddedBuffer = nullptr;\n> > > > >\n> > > > > so I don't see how it can leave embeddedBuffer unset if it returns\n> > > > > successfully.\n> > > > >\n> > > > > > Without this change, libcamera v0.2.0 usually crashes with an\n> assertion\n> > > > > > error:\n> > > > > >\n> > > > > > ipa_base.cpp:397 assertion \"it != buffers_.end()\" failed in\n> prepareIsp()\n> > > > >\n> > > > > I've seen this indeed, and will test your patch. Maybe it's due to\n> the\n> > > > > second change, where you set params.buffers.embedded to 0 ?\n> > > >\n> > > > Correct: the fix is the second change;\n> > >\n> > > Could you please update the commit message to reflect this ? I read it\n> > > as indicating that embeddedBuffer could be null, while the issue is\n> that\n> > > params.buffers.embedded is not initialized.\n> > >\n> > > > the first change is defensive in case\n> > > > findMatchBuffers for some reason doesn't set embeddedBuffer in case\n> none\n> > > > is found.\n> > >\n> > > I'm not sure I'd keep that change. I'll let Naush and David decide what\n> > > they like best.\n> >\n> > I agree, let's just keep the change to initialising\n> params.buffers.embedded.\n> > With this, the change itself looks reasonable so\n> >\n> > Reviewed-by: Naushir Patuck \n>\n> Elias, would you be able to send a v2 ?\n>\n> > However... I could have sworn the mojom auto generated class file for\n> > ipa::RPi::PrepareParams would initialize all member variables to 0\n> (including\n> > buffers.embedded) on default construction. I'm away right now so I\n> cannot\n> > check this myself, but could anybody else check? I'm somewhat surprised\n> that\n> > this has not been reported more widely...\n>\n> I checked, and the constructor doesn't initialize all fields to 0. I'm\n> pretty surprised too to be honest.\n>\n\nCuriosity got the better of me. In the\nhttps://git.libcamera.org/libcamera/libcamera.git/ build tree,\nthe following bit of code gets generated in raspberrpyi_ipa_interface.h:\n\n---\nstruct BufferIds\n{\npublic:\n#ifndef __DOXYGEN__\n BufferIds()\n {\n }\n\n BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats)\n : bayer(_bayer), embedded(_embedded), stats(_stats)\n {\n }\n#endif\n\n uint32_t bayer;\n uint32_t embedded;\n uint32_t stats;\n};\n---\n\nHowever, building from https://github.com/raspberrypi/libcamera/ generates\nthe following:\n\n---\nstruct BufferIds\n{\npublic:\n#ifndef __DOXYGEN__\n BufferIds()\n : bayer(0), embedded(0), stats(0)\n {\n }\n\n BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats)\n : bayer(_bayer), embedded(_embedded), stats(_stats)\n {\n }\n#endif\n\n uint32_t bayer;\n uint32_t embedded;\n uint32_t stats;\n};\n---\n\nSo at some point in time, the fields were explicitly initialised to 0 and\nthings were working as expected. The RPi tree definitely does not have any\nchanges to the mojom generation code, so I can't say what caused this\nchange, but it should hopefully be easy enough to bisect and find the root\ncase.\n\nRegards,\nNaush\n\n\n\n\n>\n> > > > > > Signed-off-by: Elias Naur \n> > > > > > ---\n> > > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-\n> > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > > index 26102ea7..d76389f3 100644\n> > > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n> > > > > > @@ -906,7 +906,7 @@ void\n> Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)\n> > > > > >\n> > > > > > void Vc4CameraData::tryRunPipeline()\n> > > > > > {\n> > > > > > - FrameBuffer *embeddedBuffer;\n> > > > > > + FrameBuffer *embeddedBuffer = nullptr;\n> > > > > > BayerFrame bayerFrame;\n> > > > > >\n> > > > > > /* If any of our request or buffer queues are empty, we\n> cannot proceed. */\n> > > > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()\n> > > > > > params.requestControls = request->controls();\n> > > > > > params.ipaContext = request->sequence();\n> > > > > > params.delayContext = bayerFrame.delayContext;\n> > > > > > + params.buffers.embedded = 0;\n> > > > > >\n> > > > > > if (embeddedBuffer) {\n> > > > > > unsigned int embeddedId =\n> unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\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 3D4E7BDC71\n\tfor ;\n\tWed, 24 Jan 2024 09:17:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5482362949;\n\tWed, 24 Jan 2024 10:17:22 +0100 (CET)","from mail-yb1-xb30.google.com (mail-yb1-xb30.google.com\n\t[IPv6:2607:f8b0:4864:20::b30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B752E628B6\n\tfor ;\n\tWed, 24 Jan 2024 10:17:19 +0100 (CET)","by mail-yb1-xb30.google.com with SMTP id\n\t3f1490d57ef6-dc256e97e0aso3338471276.2\n\tfor ;\n\tWed, 24 Jan 2024 01:17:19 -0800 (PST)"],"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=\"S/7Jyz8L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1706087838; x=1706692638;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=YpEhh8g6fHaJLEy9IGXS4eKmiy0E4xU3VA90NgPTBrQ=;\n\tb=S/7Jyz8LuZPdSxJsYStL/u81MG7hDy9QJsLSQLNaHuh7S3GsDFHZEW56+zqhQ2qd1k\n\tHNjUcJF3qaeYWh9PEEF/GOiFPEratCzLJXqITky9J3CS5pxnbexGBd5/EUDVc/KdU0Lr\n\tsV87Vzw+V+7JKVuDONyLepql9GBvnVFF/qReRt1XhnTwWI1cEjC+GVDdz+ZMBgArukYX\n\tYuxGUXbUW4ti8evXfgZ3FdVOK0xpjOqcwBLsHOZUqfNAV7ltRsK+4lZZ2Bg3fsHfOset\n\tqDwW3c+towSERmUGMq9xWetudhpHSr/05kfcFumF39NaN3iRhgoAXsXDYgpH1fQbX+xk\n\tV7sA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1706087838; x=1706692638;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=YpEhh8g6fHaJLEy9IGXS4eKmiy0E4xU3VA90NgPTBrQ=;\n\tb=iNsH/eegH8JJz/yZ3mSajGZxwmsEqjHIg5dTBEYO4UqTzIFhBBoYhKANeTNt9GWvE2\n\tGZbPW526YZVN7z9Y521+BK+mxSt9whf6XG9c9MWJ6xljHvQ+LtdPjGxncnoXDGchhnDF\n\tuNTJBoehiG922yjxxcwO5wCes2rP5hfHZCYgOromA6TGPbw3hgLds1xQ7bU6Jf39Pdu0\n\tEk0LsTQr8GIJpcXqytW0nMEhiCB7q2o9FG69jqwYBsOO1MccR9GIhdYiij6Mja/zoFgv\n\tuhRG2/9R6xyI2jY5UoMkO6TaCtWUZFQ387wxIEzRWM9miEmL/dwq7eTkv6lH/8Nj78+e\n\tB66w==","X-Gm-Message-State":"AOJu0YyIOVWazdGXojilA0zimHdl/Np1drVJBFE3Cp7K09509xyG2965\n\tjPN4HiK4iAe11bDTA3QxAEtv8PjvCCFdZx7OMuezCrG84yv5HN6m/9EiVxV6fohPaJ22zj1hj1Q\n\tHa4Vp80Az0m9yw4AA37VYB5Hy0e6tXtlnkEP+vQ==","X-Google-Smtp-Source":"AGHT+IEYmH18p5xeDIz33JWpblSBdZjRs/rgu7xPZMqo4DipZvjgwYwiRe0H9lEjV5lVAs2ZUFkkgWtzrjeVEaoLs3k=","X-Received":"by 2002:a25:acdf:0:b0:dc2:4450:92f2 with SMTP id\n\tx31-20020a25acdf000000b00dc2445092f2mr349021ybd.22.1706087838190;\n\tWed, 24 Jan 2024 01:17:18 -0800 (PST)","MIME-Version":"1.0","References":"<20240120211508.14742-1-mail@eliasnaur.com>\n\t<20240121040725.GB4378@pendragon.ideasonboard.com>\n\t\n\t<20240122073732.GF4378@pendragon.ideasonboard.com>\n\t\n\t<20240122103107.GO4378@pendragon.ideasonboard.com>","In-Reply-To":"<20240122103107.GO4378@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Wed, 24 Jan 2024 09:17:11 +0000","Message-ID":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000008d2731060fad8742\"","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, Elias Naur ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28620,"web_url":"https://patchwork.libcamera.org/comment/28620/","msgid":"","date":"2024-01-24T09:32:42","subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Wed, 24 Jan 2024 at 09:17, Naushir Patuck wrote:\n\n>\n>\n> On Mon, 22 Jan 2024 at 10:31, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n>> On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote:\n>> > On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel\n>> wrote:\n>> > > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote:\n>> > > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote:\n>> > > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via\n>> libcamera-devel wrote:\n>> > > > > > Vc4CameraData::findMatchBuffers may return successfully without\n>> setting\n>> > > > > > the embedded buffer. Make sure to initialize the buffer and id\n>> to avoid\n>> > > > > > accessing garbage data.\n>> > > > >\n>> > > > > How so ? The function starts with\n>> > > > >\n>> > > > > if (bayerQueue_.empty())\n>> > > > > return false;\n>> > > > >\n>> > > > > /*\n>> > > > > * Find the embedded data buffer with a matching\n>> timestamp to pass to\n>> > > > > * the IPA. Any embedded buffers with a timestamp lower\n>> than the\n>> > > > > * current bayer buffer will be removed and re-queued to\n>> the driver.\n>> > > > > */\n>> > > > > uint64_t ts =\n>> bayerQueue_.front().buffer->metadata().timestamp;\n>> > > > > embeddedBuffer = nullptr;\n>> > > > >\n>> > > > > so I don't see how it can leave embeddedBuffer unset if it returns\n>> > > > > successfully.\n>> > > > >\n>> > > > > > Without this change, libcamera v0.2.0 usually crashes with an\n>> assertion\n>> > > > > > error:\n>> > > > > >\n>> > > > > > ipa_base.cpp:397 assertion \"it != buffers_.end()\" failed in\n>> prepareIsp()\n>> > > > >\n>> > > > > I've seen this indeed, and will test your patch. Maybe it's due\n>> to the\n>> > > > > second change, where you set params.buffers.embedded to 0 ?\n>> > > >\n>> > > > Correct: the fix is the second change;\n>> > >\n>> > > Could you please update the commit message to reflect this ? I read it\n>> > > as indicating that embeddedBuffer could be null, while the issue is\n>> that\n>> > > params.buffers.embedded is not initialized.\n>> > >\n>> > > > the first change is defensive in case\n>> > > > findMatchBuffers for some reason doesn't set embeddedBuffer in case\n>> none\n>> > > > is found.\n>> > >\n>> > > I'm not sure I'd keep that change. I'll let Naush and David decide\n>> what\n>> > > they like best.\n>> >\n>> > I agree, let's just keep the change to initialising\n>> params.buffers.embedded.\n>> > With this, the change itself looks reasonable so\n>> >\n>> > Reviewed-by: Naushir Patuck \n>>\n>> Elias, would you be able to send a v2 ?\n>>\n>> > However... I could have sworn the mojom auto generated class file for\n>> > ipa::RPi::PrepareParams would initialize all member variables to 0\n>> (including\n>> > buffers.embedded) on default construction. I'm away right now so I\n>> cannot\n>> > check this myself, but could anybody else check? I'm somewhat\n>> surprised that\n>> > this has not been reported more widely...\n>>\n>> I checked, and the constructor doesn't initialize all fields to 0. I'm\n>> pretty surprised too to be honest.\n>>\n>\n> Curiosity got the better of me. In the\n> https://git.libcamera.org/libcamera/libcamera.git/ build tree,\n> the following bit of code gets generated in raspberrpyi_ipa_interface.h:\n>\n> ---\n> struct BufferIds\n> {\n> public:\n> #ifndef __DOXYGEN__\n> BufferIds()\n> {\n> }\n>\n> BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats)\n> : bayer(_bayer), embedded(_embedded), stats(_stats)\n> {\n> }\n> #endif\n>\n> uint32_t bayer;\n> uint32_t embedded;\n> uint32_t stats;\n> };\n> ---\n>\n> However, building from https://github.com/raspberrypi/libcamera/\n> generates the following:\n>\n> ---\n> struct BufferIds\n> {\n> public:\n> #ifndef __DOXYGEN__\n> BufferIds()\n> : bayer(0), embedded(0), stats(0)\n> {\n> }\n>\n> BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats)\n> : bayer(_bayer), embedded(_embedded), stats(_stats)\n> {\n> }\n> #endif\n>\n> uint32_t bayer;\n> uint32_t embedded;\n> uint32_t stats;\n> };\n> ---\n>\n> So at some point in time, the fields were explicitly initialised to 0 and\n> things were working as expected. The RPi tree definitely does not have any\n> changes to the mojom generation code, so I can't say what caused this\n> change, but it should hopefully be easy enough to bisect and find the root\n> case.\n>\n\nAnd that points to the commit:\n\ncommit d17de86904f03f1d5a4d5d20af518e70c4758969\nAuthor: Laurent Pinchart \nDate: Thu Jan 4 17:15:48 2024 +0200\n\n utils: ipc: Update mojo\n\n Update mojo from commit\n\n 9be4263648d7d1a04bb78be75df53f56449a5e3a \"Updating trunk VERSION from\n6225.0 to 6226.0\"\n\n from the Chromium repository.\n\n The update-mojo.sh script was used for this update.\n\n Bug: https://bugs.libcamera.org/show_bug.cgi?id=206\n Signed-off-by: Laurent Pinchart \n Reviewed-by: Milan Zamazal \n Signed-off-by: Kieran Bingham \n\n\n\n>\n> Regards,\n> Naush\n>\n>\n>\n>\n>>\n>> > > > > > Signed-off-by: Elias Naur \n>> > > > > > ---\n>> > > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-\n>> > > > > > 1 file changed, 2 insertions(+), 1 deletion(-)\n>> > > > > >\n>> > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n>> b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n>> > > > > > index 26102ea7..d76389f3 100644\n>> > > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n>> > > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp\n>> > > > > > @@ -906,7 +906,7 @@ void\n>> Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)\n>> > > > > >\n>> > > > > > void Vc4CameraData::tryRunPipeline()\n>> > > > > > {\n>> > > > > > - FrameBuffer *embeddedBuffer;\n>> > > > > > + FrameBuffer *embeddedBuffer = nullptr;\n>> > > > > > BayerFrame bayerFrame;\n>> > > > > >\n>> > > > > > /* If any of our request or buffer queues are empty, we\n>> cannot proceed. */\n>> > > > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()\n>> > > > > > params.requestControls = request->controls();\n>> > > > > > params.ipaContext = request->sequence();\n>> > > > > > params.delayContext = bayerFrame.delayContext;\n>> > > > > > + params.buffers.embedded = 0;\n>> > > > > >\n>> > > > > > if (embeddedBuffer) {\n>> > > > > > unsigned int embeddedId =\n>> unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\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 0DD81C323E\n\tfor ;\n\tWed, 24 Jan 2024 09:32:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3333462949;\n\tWed, 24 Jan 2024 10:32:52 +0100 (CET)","from mail-yb1-xb29.google.com (mail-yb1-xb29.google.com\n\t[IPv6:2607:f8b0:4864:20::b29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA288628B6\n\tfor ;\n\tWed, 24 Jan 2024 10:32:49 +0100 (CET)","by mail-yb1-xb29.google.com with SMTP id\n\t3f1490d57ef6-dc238cb1b17so4825696276.0\n\tfor ;\n\tWed, 24 Jan 2024 01:32:49 -0800 (PST)"],"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=\"GceD8fK1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1706088768; x=1706693568;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=WltyxIRSkIc3M7lwuUGInzf5zOhSVLvl9IOj0co94dc=;\n\tb=GceD8fK16o+HSsAaUmTejYsaarY9E0RAQdtYIc6sQhNVH2T6cNI0zGPNwsiZi3wI79\n\tjb9TI+cHd756Fgh82dvxqMq7iETnlqnUzW3IOmMGSh7Gh0XpCSEnbd0pPT2D6WOBHorH\n\t/Lcjl4djLIIQqNdbH4Ng3jj3rGcH32xoh3n2VSpONDx7wgszZfyY0HnVkmOESAQxWHM2\n\t8JEpHp2mLJRl+2hmDYQGrsmMp0QVn1uFFvOgrZXOn1Mq+iLDTqi+4SM9IlMI5gDXj9JY\n\tpJ6rDphakCl9vWzKMx3qJCnBukDDWTsFjQ/0o7OXagINGXU3yy7TedchEHJfnHckxehB\n\tjUsQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1706088768; x=1706693568;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=WltyxIRSkIc3M7lwuUGInzf5zOhSVLvl9IOj0co94dc=;\n\tb=To8tweV00jnUly/tnLkRrpw5uhrNcHbfU+q6EtV13mm7pIoCP6Zi+Z0wx6cMR6vyNJ\n\tbpuJDF0YoAq0WElx6LCJtBzcmOS3vI0StUYSQE6IRXYKgQM4rQDou2kxIRinL/X4WqpB\n\tnmpj2pcKt/+vum5glc8SfzRVTmGHhNnFABCkywkX37K0sVfQfGrRrX24YuwHIcqcWxuP\n\t7NDVGlCV4bSWootkf4puJSOox9H/81e8xiYWYbRuA2ZfCU3BccUk6kSc0b4rnXKapFkS\n\thDntsxX331bQGTKukvyZFJHrWolzyUGGrq6QDOmSEeKfuKMSl0Y+I0y3uIxukxwyD8lQ\n\t+Ghw==","X-Gm-Message-State":"AOJu0YymEOUI6yaGkWmRiHjaIQ3szwmz4ka2XEeSkHFpjP/Fv/4VRr5I\n\ta/WgjK9DTpkjG/z7U/jrb+xI4Z06gQqGTHd9C4Hvr7dVzeHoW+9fAYn5ZHgrXABDo+sQ4/7WL+z\n\t00VtzwCg3mkN5vh5PEJI1Xy9MssBvYjbH4GoQxg==","X-Google-Smtp-Source":"AGHT+IFcJjzLMP0Aet6yfyxlgDF7ib8OTV/bqOmRXUiijO9z+2ILmXWBrR+RbR1aoHhHJzVh+af4vRBVXhCsGDZv92Q=","X-Received":"by 2002:a25:8047:0:b0:dc2:544f:cfb4 with SMTP id\n\ta7-20020a258047000000b00dc2544fcfb4mr322749ybn.16.1706088768597;\n\tWed, 24 Jan 2024 01:32:48 -0800 (PST)","MIME-Version":"1.0","References":"<20240120211508.14742-1-mail@eliasnaur.com>\n\t<20240121040725.GB4378@pendragon.ideasonboard.com>\n\t\n\t<20240122073732.GF4378@pendragon.ideasonboard.com>\n\t\n\t<20240122103107.GO4378@pendragon.ideasonboard.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 24 Jan 2024 09:32:42 +0000","Message-ID":"","Subject":"Re: [libcamera-devel] [PATCH] pipeline: rpi: always initialize the\n\tembedded buffer in tryRunPipeline","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000000201d1060fadbf85\"","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, Elias Naur ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]