[{"id":23243,"web_url":"https://patchwork.libcamera.org/comment/23243/","msgid":"<20220530095655.mlagqbi2qcae6zfn@uno.localdomain>","date":"2022-05-30T09:56:55","subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul\n\nOn Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote:\n> From: Jacopo Mondi \n>\n> Mapped streams are generated by post-processing and always require a\n> source buffer from where to process image data from.\n>\n> In case a Mapped stream is requested but its source stream is not, it\n> is required to allocate a buffer on the fly and add it to the\n> libcamera::Request.\n>\n> Make sure a source stream is available for all mapped streams, and if\n> that's not the case, add a dedicated buffer to the request for that\n> purpose.\n>\n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v2:\n> - cosmetic changes in code\n> - fix typo in the commit message\n> ---\n> src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++----\n> 1 file changed, 72 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 20599665..95c14f60 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -9,6 +9,7 @@\n>\n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tLOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n> \t\t\t<< \" with \" << descriptor->buffers_.size() << \" streams\";\n>\n> +\t/*\n> +\t * Collect the CameraStream associated to each requested capture stream.\n> +\t * Since requestedStreams is an std:set<>, no duplications can happen.\n std::set<>\n\n> +\t */\n> +\tstd::set requestedStreams;\n> +\tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> +\t\tCameraStream *cameraStream = buffer.stream;\n> +\n> +\t\tswitch (cameraStream->type()) {\n> +\t\tcase CameraStream::Type::Mapped:\n> +\t\t\trequestedStreams.insert(cameraStream->sourceStream());\n> +\t\t\tbreak;\n> +\n> +\t\tcase CameraStream::Type::Direct:\n> +\t\tcase CameraStream::Type::Internal:\n> +\t\t\trequestedStreams.insert(cameraStream);\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\t/*\n> +\t * Process all the Direct and Internal streams, for which the CameraStream\n> +\t * they refer to is the one that points to the right libcamera::Stream.\n> +\t *\n> +\t * Streams of type Mapped will be handled later.\n> +\t */\n> \tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> \t\tCameraStream *cameraStream = buffer.stream;\n> \t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>\n> \t\tswitch (cameraStream->type()) {\n> \t\tcase CameraStream::Type::Mapped:\n> -\t\t\t/*\n> -\t\t\t * Mapped streams don't need buffers added to the\n> -\t\t\t * Request.\n> -\t\t\t */\n> -\t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n> -\n> -\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> -\t\t\t\t{ cameraStream, &buffer });\n> \t\t\tcontinue;\n>\n> \t\tcase CameraStream::Type::Direct:\n> @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tauto fence = std::make_unique(std::move(acquireFence));\n> \t\tdescriptor->request_->addBuffer(cameraStream->stream(),\n> \t\t\t\t\t\tframeBuffer, std::move(fence));\n> +\n> +\t\trequestedStreams.erase(cameraStream);\n> +\t}\n> +\n> +\t/*\n> +\t * Now handle the Mapped streams. If no buffer has been added for them\n> +\t * as their corresponding direct source stream has not been requested,\n> +\t * add it here.\n> +\t */\n> +\tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> +\t\tCameraStream *cameraStream = buffer.stream;\n> +\t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> +\n> +\t\tif (cameraStream->type() != CameraStream::Type::Mapped)\n> +\t\t\tcontinue;\n> +\n> +\t\tLOG(HAL, Debug) << i << \" - (\" << camera3Stream->width << \"x\"\n> +\t\t\t\t<< camera3Stream->height << \")\"\n> +\t\t\t\t<< \"[\" << utils::hex(camera3Stream->format) << \"] -> \"\n> +\t\t\t\t<< \"(\" << cameraStream->configuration().size.toString() << \")[\"\n> +\t\t\t\t<< cameraStream->configuration().pixelFormat.toString() << \"]\"\n> +\t\t\t\t<< \" (mapped)\";\n> +\n> +\t\tMutexLocker lock(descriptor->streamsProcessMutex_);\n> +\t\tdescriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });\n> +\n> +\t\t/*\n> +\t\t * Make sure the CameraStream this stream is mapped on has been\n> +\t\t * added to the request.\n> +\t\t */\n> +\t\tCameraStream *sourceStream = cameraStream->sourceStream();\n> +\t\tif (requestedStreams.find(sourceStream) == requestedStreams.end())\n> +\t\t\tcontinue;\n> +\n> +\t\t/*\n> +\t\t * If that's not the case, we need to add a buffer to the request\n> +\t\t * for this stream.\n> +\t\t */\n> +\t\tFrameBuffer *frameBuffer = cameraStream->getBuffer();\n> +\t\tbuffer.internalBuffer = frameBuffer;\n> +\n> +\t\tdescriptor->request_->addBuffer(sourceStream->stream(),\n> +\t\t\t\t\t\tframeBuffer, nullptr);\n> +\n> +\t\trequestedStreams.erase(sourceStream);\n> \t}\n\nWe could possibily make sure here that now requestedStreams is empty,\nbut that's just an additional safety check.\n\nThe patch still looks ok to me.\n\n>\n> \t/*\n> --\n> 2.30.2\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 A0BFFBD160\n\tfor ;\n\tMon, 30 May 2022 09:57:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B80261FB7;\n\tMon, 30 May 2022 11:57:00 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::221])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4ADD66040B\n\tfor ;\n\tMon, 30 May 2022 11:56:58 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 655AC240005;\n\tMon, 30 May 2022 09:56:57 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653904620;\n\tbh=ANhVu7KSLIJ1YXgTG8AWwSYT2yDHvf9lIqZbLsobv4U=;\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=KPJkJhGthADSHi5DdaMrQM1C0u4qxU1721p9JhGuzQZTyNBU1HJgxycdUP1xLkG0n\n\tKBZXYY6M2sy7E9QqAuFaS6WJ+6iN8Itq06ljt420I1wfeDTbbkoaOzghcdf6g/tzyK\n\tRuLC2gATCHutkvQ6jtc3IwV041TBECbiUD1UTaJToOY2AN5O59wB/qUwBRLsm8FTtt\n\tvDZnA70UN/YZMi32aaNCfQ/giruNPKPXAEU7trJh7qvrZ+sh1+SU24cs+otSz8tM3Q\n\trCTKEJRxgHmmYZHKwSwW8DgG76aSimiprFrOzcdD6/vRJEuaXoPWJnfC1qxMYtBRLr\n\tnByD8WfE2w0kw==","Date":"Mon, 30 May 2022 11:56:55 +0200","To":"Paul Elder ","Message-ID":"<20220530095655.mlagqbi2qcae6zfn@uno.localdomain>","References":"<20220527093440.953377-1-paul.elder@ideasonboard.com>\n\t<20220527093440.953377-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220527093440.953377-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23250,"web_url":"https://patchwork.libcamera.org/comment/23250/","msgid":"<9eb7f714-8d9d-59e1-2924-02d5e253b521@ideasonboard.com>","date":"2022-05-30T13:41:20","subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote:\n> Hi Paul\n>\n> On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote:\n>> From: Jacopo Mondi \n>>\n>> Mapped streams are generated by post-processing and always require a\n>> source buffer from where to process image data from.\n>>\n>> In case a Mapped stream is requested but its source stream is not, it\n>> is required to allocate a buffer on the fly and add it to the\n>> libcamera::Request.\n>>\n>> Make sure a source stream is available for all mapped streams, and if\n>> that's not the case, add a dedicated buffer to the request for that\n>> purpose.\n>>\n>> Signed-off-by: Jacopo Mondi \n>> Signed-off-by: Paul Elder \n>>\n>> ---\n>> Changes in v2:\n>> - cosmetic changes in code\n>> - fix typo in the commit message\n>> ---\n>> src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++----\n>> 1 file changed, 72 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 20599665..95c14f60 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -9,6 +9,7 @@\n>>\n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>> #include \n>> @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \tLOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n>> \t\t\t<< \" with \" << descriptor->buffers_.size() << \" streams\";\n>>\n>> +\t/*\n>> +\t * Collect the CameraStream associated to each requested capture stream.\n>> +\t * Since requestedStreams is an std:set<>, no duplications can happen.\n> std::set<>\n>\n>> +\t */\n>> +\tstd::set requestedStreams;\n>> +\tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n>> +\t\tCameraStream *cameraStream = buffer.stream;\n>> +\n>> +\t\tswitch (cameraStream->type()) {\n>> +\t\tcase CameraStream::Type::Mapped:\n>> +\t\t\trequestedStreams.insert(cameraStream->sourceStream());\n\n\nCan sourceStream field for Mapped streams be nullptr here? Should we \nensure it via an ASSERT?\n\n>> +\t\t\tbreak;\n>> +\n>> +\t\tcase CameraStream::Type::Direct:\n>> +\t\tcase CameraStream::Type::Internal:\n>> +\t\t\trequestedStreams.insert(cameraStream);\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Process all the Direct and Internal streams, for which the CameraStream\n>> +\t * they refer to is the one that points to the right libcamera::Stream.\n>> +\t *\n>> +\t * Streams of type Mapped will be handled later.\n>> +\t */\n>> \tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n>> \t\tCameraStream *cameraStream = buffer.stream;\n>> \t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n>> @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>\n>> \t\tswitch (cameraStream->type()) {\n>> \t\tcase CameraStream::Type::Mapped:\n>> -\t\t\t/*\n>> -\t\t\t * Mapped streams don't need buffers added to the\n>> -\t\t\t * Request.\n>> -\t\t\t */\n>> -\t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n>> -\n>> -\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n>> -\t\t\t\t{ cameraStream, &buffer });\n>> \t\t\tcontinue;\n>>\n>> \t\tcase CameraStream::Type::Direct:\n>> @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\tauto fence = std::make_unique(std::move(acquireFence));\n>> \t\tdescriptor->request_->addBuffer(cameraStream->stream(),\n>> \t\t\t\t\t\tframeBuffer, std::move(fence));\n>> +\n>> +\t\trequestedStreams.erase(cameraStream);\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Now handle the Mapped streams. If no buffer has been added for them\n>> +\t * as their corresponding direct source stream has not been requested,\n>> +\t * add it here.\n\n\nI am wondering a situation where a direct stream D, and a mapped stream \nM, is requested and M is mapped onto D so,\n\n         M->sourceStream = D;\n\nProvided the requestedStreams is a std::set<> where no duplications can \nhappen, and given the above scenario:\nI see the requestedStreams will consist of  { D } while populating the \nset in the switch-case above, which then  gets\nerased from the requestedStreams (above the comment block) so now, the \nrequestedStreams become an empty set { } here . . .\n\n>> +\t */\n>> +\tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n>> +\t\tCameraStream *cameraStream = buffer.stream;\n>> +\t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n>> +\n>> +\t\tif (cameraStream->type() != CameraStream::Type::Mapped)\n>> +\t\t\tcontinue;\n>> +\n>> +\t\tLOG(HAL, Debug) << i << \" - (\" << camera3Stream->width << \"x\"\n>> +\t\t\t\t<< camera3Stream->height << \")\"\n>> +\t\t\t\t<< \"[\" << utils::hex(camera3Stream->format) << \"] -> \"\n>> +\t\t\t\t<< \"(\" << cameraStream->configuration().size.toString() << \")[\"\n>> +\t\t\t\t<< cameraStream->configuration().pixelFormat.toString() << \"]\"\n>> +\t\t\t\t<< \" (mapped)\";\n>> +\n>> +\t\tMutexLocker lock(descriptor->streamsProcessMutex_);\n>> +\t\tdescriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });\n>> +\n>> +\t\t/*\n>> +\t\t * Make sure the CameraStream this stream is mapped on has been\n>> +\t\t * added to the request.\n>> +\t\t */\n>> +\t\tCameraStream *sourceStream = cameraStream->sourceStream();\n>> +\t\tif (requestedStreams.find(sourceStream) == requestedStreams.end())\n\n\nand then while handling mapped streams, it will try to find { D }  in \nrequestedStreams (which is now empty)\n\n>> +\t\t\tcontinue;\n>> +\n>> +\t\t/*\n>> +\t\t * If that's not the case, we need to add a buffer to the request\n>> +\t\t * for this stream.\n>> +\t\t */\n>> +\t\tFrameBuffer *frameBuffer = cameraStream->getBuffer();\n>> +\t\tbuffer.internalBuffer = frameBuffer;\n>> +\n>> +\t\tdescriptor->request_->addBuffer(sourceStream->stream(),\n>> +\t\t\t\t\t\tframeBuffer, nullptr);\n\n\n... and add a internal buffer for D\n\nso we have 2 buffers for D in the end ?\n\n>> +\n>> +\t\trequestedStreams.erase(sourceStream);\n>> \t}\n> We could possibily make sure here that now requestedStreams is empty,\n> but that's just an additional safety check.\n>\n> The patch still looks ok to me.\n>\n>> \t/*\n>> --\n>> 2.30.2\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 0C309BD160\n\tfor ;\n\tMon, 30 May 2022 13:41:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 20B2065633;\n\tMon, 30 May 2022 15:41:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3514A60411\n\tfor ;\n\tMon, 30 May 2022 15:41:24 +0200 (CEST)","from [192.168.1.68] (235.red-81-36-186.dynamicip.rima-tde.net\n\t[81.36.186.235])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 681E7B90;\n\tMon, 30 May 2022 15:41:23 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653918086;\n\tbh=ezH9CJIqs2mdovWAf2mfHPcRxMSfcMVTCrtXbCBvEk0=;\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=nb9BCJ9IyxjsDjv6YpREuwK+rNXnujGPTByYE9ITSvQQ38v1nC9EV+2eM+nfy+fgr\n\tyBJ0yXnqS7ussqSicvUF/ls0c+V/xHinapE7RweE+utjFFq05xihKEgyVsD/uAQW2C\n\tJzhbZxelXSupndedPBZoZdHv4zgn2qXE+azRNQ+G/A3np5lqe5Y3kU+y0mAqpHvAgp\n\tGKN+awh1HfGvpCR+LBpuDSaGQf4cBmstO8jfsQwY+QcJKcpPO6p5nPgdLv2Were0rH\n\tmRGgf/2JS4xWYZoSvJCxBSuwhbbjaaS1SP5AQsjIFWLfvj+ZZXkrbtStgk8XdL3tuW\n\tR1uHKVPv/lZdw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653918083;\n\tbh=ezH9CJIqs2mdovWAf2mfHPcRxMSfcMVTCrtXbCBvEk0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=I8bJ5kYP4W5TOvTU6xEC3KE4sNOhzaa7sHQeh8NAxtImSvJOClbOtnpBL87ICIwsr\n\tMfu8EdD6eGbMbJ3mu1nyTh2XBpMfXI/1Uh0RMWTNkIcVTJ8ROhkYTePpDu8G8g01w8\n\t2CdIHFEfkwdiS1NRDKUezRnvLO0siTe0oxkt5W90="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"I8bJ5kYP\"; dkim-atps=neutral","Message-ID":"<9eb7f714-8d9d-59e1-2924-02d5e253b521@ideasonboard.com>","Date":"Mon, 30 May 2022 15:41:20 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Jacopo Mondi ,\n\tPaul Elder ","References":"<20220527093440.953377-1-paul.elder@ideasonboard.com>\n\t<20220527093440.953377-4-paul.elder@ideasonboard.com>\n\t<20220530095655.mlagqbi2qcae6zfn@uno.localdomain>","In-Reply-To":"<20220530095655.mlagqbi2qcae6zfn@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23251,"web_url":"https://patchwork.libcamera.org/comment/23251/","msgid":"<20220530142705.72tsrmn63hcw7xqr@uno.localdomain>","date":"2022-05-30T14:27:05","subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Mon, May 30, 2022 at 03:41:20PM +0200, Umang Jain wrote:\n> Hi Jacopo,\n>\n> On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote:\n> > Hi Paul\n> >\n> > On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote:\n> > > From: Jacopo Mondi \n> > >\n> > > Mapped streams are generated by post-processing and always require a\n> > > source buffer from where to process image data from.\n> > >\n> > > In case a Mapped stream is requested but its source stream is not, it\n> > > is required to allocate a buffer on the fly and add it to the\n> > > libcamera::Request.\n> > >\n> > > Make sure a source stream is available for all mapped streams, and if\n> > > that's not the case, add a dedicated buffer to the request for that\n> > > purpose.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > Changes in v2:\n> > > - cosmetic changes in code\n> > > - fix typo in the commit message\n> > > ---\n> > > src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++----\n> > > 1 file changed, 72 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 20599665..95c14f60 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > \tLOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n> > > \t\t\t<< \" with \" << descriptor->buffers_.size() << \" streams\";\n> > >\n> > > +\t/*\n> > > +\t * Collect the CameraStream associated to each requested capture stream.\n> > > +\t * Since requestedStreams is an std:set<>, no duplications can happen.\n> > std::set<>\n> >\n> > > +\t */\n> > > +\tstd::set requestedStreams;\n> > > +\tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> > > +\t\tCameraStream *cameraStream = buffer.stream;\n> > > +\n> > > +\t\tswitch (cameraStream->type()) {\n> > > +\t\tcase CameraStream::Type::Mapped:\n> > > +\t\t\trequestedStreams.insert(cameraStream->sourceStream());\n>\n>\n> Can sourceStream field for Mapped streams be nullptr here? Should we ensure\n> it via an ASSERT?\n>\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tcase CameraStream::Type::Direct:\n> > > +\t\tcase CameraStream::Type::Internal:\n> > > +\t\t\trequestedStreams.insert(cameraStream);\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Process all the Direct and Internal streams, for which the CameraStream\n> > > +\t * they refer to is the one that points to the right libcamera::Stream.\n> > > +\t *\n> > > +\t * Streams of type Mapped will be handled later.\n> > > +\t */\n> > > \tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> > > \t\tCameraStream *cameraStream = buffer.stream;\n> > > \t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> > > @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >\n> > > \t\tswitch (cameraStream->type()) {\n> > > \t\tcase CameraStream::Type::Mapped:\n> > > -\t\t\t/*\n> > > -\t\t\t * Mapped streams don't need buffers added to the\n> > > -\t\t\t * Request.\n> > > -\t\t\t */\n> > > -\t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n> > > -\n> > > -\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n> > > -\t\t\t\t{ cameraStream, &buffer });\n> > > \t\t\tcontinue;\n> > >\n> > > \t\tcase CameraStream::Type::Direct:\n> > > @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > \t\tauto fence = std::make_unique(std::move(acquireFence));\n> > > \t\tdescriptor->request_->addBuffer(cameraStream->stream(),\n> > > \t\t\t\t\t\tframeBuffer, std::move(fence));\n> > > +\n> > > +\t\trequestedStreams.erase(cameraStream);\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Now handle the Mapped streams. If no buffer has been added for them\n> > > +\t * as their corresponding direct source stream has not been requested,\n> > > +\t * add it here.\n>\n>\n> I am wondering a situation where a direct stream D, and a mapped stream M,\n> is requested and M is mapped onto D so,\n>\n>         M->sourceStream = D;\n>\n> Provided the requestedStreams is a std::set<> where no duplications can\n> happen, and given the above scenario:\n> I see the requestedStreams will consist of  { D } while populating the set\n> in the switch-case above, which then  gets\n> erased from the requestedStreams (above the comment block) so now, the\n> requestedStreams become an empty set { } here . . .\n>\n> > > +\t */\n> > > +\tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> > > +\t\tCameraStream *cameraStream = buffer.stream;\n> > > +\t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> > > +\n> > > +\t\tif (cameraStream->type() != CameraStream::Type::Mapped)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tLOG(HAL, Debug) << i << \" - (\" << camera3Stream->width << \"x\"\n> > > +\t\t\t\t<< camera3Stream->height << \")\"\n> > > +\t\t\t\t<< \"[\" << utils::hex(camera3Stream->format) << \"] -> \"\n> > > +\t\t\t\t<< \"(\" << cameraStream->configuration().size.toString() << \")[\"\n> > > +\t\t\t\t<< cameraStream->configuration().pixelFormat.toString() << \"]\"\n> > > +\t\t\t\t<< \" (mapped)\";\n> > > +\n> > > +\t\tMutexLocker lock(descriptor->streamsProcessMutex_);\n> > > +\t\tdescriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Make sure the CameraStream this stream is mapped on has been\n> > > +\t\t * added to the request.\n> > > +\t\t */\n> > > +\t\tCameraStream *sourceStream = cameraStream->sourceStream();\n> > > +\t\tif (requestedStreams.find(sourceStream) == requestedStreams.end())\n>\n>\n> and then while handling mapped streams, it will try to find { D }  in\n> requestedStreams (which is now empty)\n>\n> > > +\t\t\tcontinue;\n\nSo that the find() operation returns == requestedStreams.end() and we\ncontinue here\n\n> > > +\n> > > +\t\t/*\n> > > +\t\t * If that's not the case, we need to add a buffer to the request\n> > > +\t\t * for this stream.\n> > > +\t\t */\n> > > +\t\tFrameBuffer *frameBuffer = cameraStream->getBuffer();\n> > > +\t\tbuffer.internalBuffer = frameBuffer;\n> > > +\n> > > +\t\tdescriptor->request_->addBuffer(sourceStream->stream(),\n> > > +\t\t\t\t\t\tframeBuffer, nullptr);\n>\n>\n> ... and add a internal buffer for D\n>\n> so we have 2 buffers for D in the end ?\n\nSo we don't get here and won't add the buffer twice.\n\nHave I missed something on you comment ?\n\n>\n> > > +\n> > > +\t\trequestedStreams.erase(sourceStream);\n> > > \t}\n> > We could possibily make sure here that now requestedStreams is empty,\n> > but that's just an additional safety check.\n> >\n> > The patch still looks ok to me.\n> >\n> > > \t/*\n> > > --\n> > > 2.30.2\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 19ACABD161\n\tfor ;\n\tMon, 30 May 2022 14:27:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D91A61FB7;\n\tMon, 30 May 2022 16:27:10 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::226])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECD8860411\n\tfor ;\n\tMon, 30 May 2022 16:27:08 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id A932FC000B;\n\tMon, 30 May 2022 14:27:07 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653920830;\n\tbh=IZbHIN/LGIJ8URZf+Np/28NMV5LjbxWLypIrWRUhIxw=;\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=vietpxIgWPDDN0qqHKEBO2F0VlrJA97dzyMoYF31IhNwKXnK5Z8STvp53CD8mZE8N\n\tLw3F68vPAqmXVbBbfcH/BMCeufXquijqEC9iW2/Drkb+Be1p07qkgtGXIPDgVl747e\n\tyZ3tVkuGIwEmBXnDMcZndGv6yqTRyu+/3cjOyNgvsKuaq7FJMrkSV4bT4tyFbcSMDM\n\t10AMgi+KgT7H59hPM/JJX8MdcYq9g/dbZnQjgcqAOJFViNIpwNcKfCS6GWn/gE9vwy\n\tF3NpPa4uf9IcD9GslfA0+xMXy+yShKwohYZXifxC0vmCJ/2j3uq7UasrjmJDFKaYHR\n\t+/FYjJ376/C9g==","Date":"Mon, 30 May 2022 16:27:05 +0200","To":"Umang Jain ","Message-ID":"<20220530142705.72tsrmn63hcw7xqr@uno.localdomain>","References":"<20220527093440.953377-1-paul.elder@ideasonboard.com>\n\t<20220527093440.953377-4-paul.elder@ideasonboard.com>\n\t<20220530095655.mlagqbi2qcae6zfn@uno.localdomain>\n\t<9eb7f714-8d9d-59e1-2924-02d5e253b521@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<9eb7f714-8d9d-59e1-2924-02d5e253b521@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23254,"web_url":"https://patchwork.libcamera.org/comment/23254/","msgid":"<528c3ba3-18ce-be13-27c4-22d8ff04de5a@ideasonboard.com>","date":"2022-05-30T19:58:34","subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 5/30/22 16:27, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Mon, May 30, 2022 at 03:41:20PM +0200, Umang Jain wrote:\n>> Hi Jacopo,\n>>\n>> On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote:\n>>> Hi Paul\n>>>\n>>> On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote:\n>>>> From: Jacopo Mondi \n>>>>\n>>>> Mapped streams are generated by post-processing and always require a\n>>>> source buffer from where to process image data from.\n>>>>\n>>>> In case a Mapped stream is requested but its source stream is not, it\n>>>> is required to allocate a buffer on the fly and add it to the\n>>>> libcamera::Request.\n>>>>\n>>>> Make sure a source stream is available for all mapped streams, and if\n>>>> that's not the case, add a dedicated buffer to the request for that\n>>>> purpose.\n>>>>\n>>>> Signed-off-by: Jacopo Mondi \n>>>> Signed-off-by: Paul Elder \n>>>>\n>>>> ---\n>>>> Changes in v2:\n>>>> - cosmetic changes in code\n>>>> - fix typo in the commit message\n>>>> ---\n>>>> src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++----\n>>>> 1 file changed, 72 insertions(+), 8 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 20599665..95c14f60 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -9,6 +9,7 @@\n>>>>\n>>>> #include \n>>>> #include \n>>>> +#include \n>>>> #include \n>>>> #include \n>>>> #include \n>>>> @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> \tLOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n>>>> \t\t\t<< \" with \" << descriptor->buffers_.size() << \" streams\";\n>>>>\n>>>> +\t/*\n>>>> +\t * Collect the CameraStream associated to each requested capture stream.\n>>>> +\t * Since requestedStreams is an std:set<>, no duplications can happen.\n>>> std::set<>\n>>>\n>>>> +\t */\n>>>> +\tstd::set requestedStreams;\n>>>> +\tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n>>>> +\t\tCameraStream *cameraStream = buffer.stream;\n>>>> +\n>>>> +\t\tswitch (cameraStream->type()) {\n>>>> +\t\tcase CameraStream::Type::Mapped:\n>>>> +\t\t\trequestedStreams.insert(cameraStream->sourceStream());\n>>\n>> Can sourceStream field for Mapped streams be nullptr here? Should we ensure\n>> it via an ASSERT?\n\n\nWould like your thoughts on this as well..\n\n>>\n>>>> +\t\t\tbreak;\n>>>> +\n>>>> +\t\tcase CameraStream::Type::Direct:\n>>>> +\t\tcase CameraStream::Type::Internal:\n>>>> +\t\t\trequestedStreams.insert(cameraStream);\n>>>> +\t\t\tbreak;\n>>>> +\t\t}\n>>>> +\t}\n>>>> +\n>>>> +\t/*\n>>>> +\t * Process all the Direct and Internal streams, for which the CameraStream\n>>>> +\t * they refer to is the one that points to the right libcamera::Stream.\n>>>> +\t *\n>>>> +\t * Streams of type Mapped will be handled later.\n>>>> +\t */\n>>>> \tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n>>>> \t\tCameraStream *cameraStream = buffer.stream;\n>>>> \t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n>>>> @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>>\n>>>> \t\tswitch (cameraStream->type()) {\n>>>> \t\tcase CameraStream::Type::Mapped:\n>>>> -\t\t\t/*\n>>>> -\t\t\t * Mapped streams don't need buffers added to the\n>>>> -\t\t\t * Request.\n>>>> -\t\t\t */\n>>>> -\t\t\tLOG(HAL, Debug) << ss.str() << \" (mapped)\";\n>>>> -\n>>>> -\t\t\tdescriptor->pendingStreamsToProcess_.insert(\n>>>> -\t\t\t\t{ cameraStream, &buffer });\n>>>> \t\t\tcontinue;\n>>>>\n>>>> \t\tcase CameraStream::Type::Direct:\n>>>> @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> \t\tauto fence = std::make_unique(std::move(acquireFence));\n>>>> \t\tdescriptor->request_->addBuffer(cameraStream->stream(),\n>>>> \t\t\t\t\t\tframeBuffer, std::move(fence));\n>>>> +\n>>>> +\t\trequestedStreams.erase(cameraStream);\n>>>> +\t}\n>>>> +\n>>>> +\t/*\n>>>> +\t * Now handle the Mapped streams. If no buffer has been added for them\n>>>> +\t * as their corresponding direct source stream has not been requested,\n>>>> +\t * add it here.\n>>\n>> I am wondering a situation where a direct stream D, and a mapped stream M,\n>> is requested and M is mapped onto D so,\n>>\n>>         M->sourceStream = D;\n>>\n>> Provided the requestedStreams is a std::set<> where no duplications can\n>> happen, and given the above scenario:\n>> I see the requestedStreams will consist of  { D } while populating the set\n>> in the switch-case above, which then  gets\n>> erased from the requestedStreams (above the comment block) so now, the\n>> requestedStreams become an empty set { } here . . .\n>>\n>>>> +\t */\n>>>> +\tfor (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n>>>> +\t\tCameraStream *cameraStream = buffer.stream;\n>>>> +\t\tcamera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n>>>> +\n>>>> +\t\tif (cameraStream->type() != CameraStream::Type::Mapped)\n>>>> +\t\t\tcontinue;\n>>>> +\n>>>> +\t\tLOG(HAL, Debug) << i << \" - (\" << camera3Stream->width << \"x\"\n>>>> +\t\t\t\t<< camera3Stream->height << \")\"\n>>>> +\t\t\t\t<< \"[\" << utils::hex(camera3Stream->format) << \"] -> \"\n>>>> +\t\t\t\t<< \"(\" << cameraStream->configuration().size.toString() << \")[\"\n>>>> +\t\t\t\t<< cameraStream->configuration().pixelFormat.toString() << \"]\"\n>>>> +\t\t\t\t<< \" (mapped)\";\n>>>> +\n>>>> +\t\tMutexLocker lock(descriptor->streamsProcessMutex_);\n>>>> +\t\tdescriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });\n>>>> +\n>>>> +\t\t/*\n>>>> +\t\t * Make sure the CameraStream this stream is mapped on has been\n>>>> +\t\t * added to the request.\n>>>> +\t\t */\n>>>> +\t\tCameraStream *sourceStream = cameraStream->sourceStream();\n>>>> +\t\tif (requestedStreams.find(sourceStream) == requestedStreams.end())\n>>\n>> and then while handling mapped streams, it will try to find { D }  in\n>> requestedStreams (which is now empty)\n>>\n>>>> +\t\t\tcontinue;\n> So that the find() operation returns == requestedStreams.end() and we\n\n\nAh .. so silly of me thinking the comparator as != requestedStreams.end()\ninstead of == requestedStreams.end(). So code patterns do stick in the \nbrain :D\n\n> continue here\n>\n>>>> +\n>>>> +\t\t/*\n>>>> +\t\t * If that's not the case, we need to add a buffer to the request\n>>>> +\t\t * for this stream.\n>>>> +\t\t */\n>>>> +\t\tFrameBuffer *frameBuffer = cameraStream->getBuffer();\n>>>> +\t\tbuffer.internalBuffer = frameBuffer;\n>>>> +\n>>>> +\t\tdescriptor->request_->addBuffer(sourceStream->stream(),\n>>>> +\t\t\t\t\t\tframeBuffer, nullptr);\n>>\n>> ... and add a internal buffer for D\n>>\n>> so we have 2 buffers for D in the end ?\n> So we don't get here and won't add the buffer twice.\n\n\nMakes sense now\n\n>\n> Have I missed something on you comment ?\n\n\nNo. It looks good to me now!\n\n>\n>>>> +\n>>>> +\t\trequestedStreams.erase(sourceStream);\n>>>> \t}\n>>> We could possibily make sure here that now requestedStreams is empty,\n>>> but that's just an additional safety check.\n>>>\n>>> The patch still looks ok to me.\n>>>\n>>>> \t/*\n>>>> --\n>>>> 2.30.2\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 163A9BD161\n\tfor ;\n\tMon, 30 May 2022 19:58:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4234465633;\n\tMon, 30 May 2022 21:58:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D73C860411\n\tfor ;\n\tMon, 30 May 2022 21:58:38 +0200 (CEST)","from [192.168.1.68] (235.red-81-36-186.dynamicip.rima-tde.net\n\t[81.36.186.235])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F27FF6DF;\n\tMon, 30 May 2022 21:58:37 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1653940721;\n\tbh=72jAKo3eeTyauBsQLTbTw6gLTJdTXFDuI9CtEAdKsQQ=;\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=p045K9QMZrxFyMTbiMW4FVuvUIKYZIQnurZC9XtclGqR857vTf5QR797T2BaDm7DV\n\tFeytwHSgviC0hLmnaSD4+CuKshVj64BXCpbQeF0LAZNrIPZjGsPLM/ZawA1d0VFrfp\n\tb4rCZdneqUJZzx+h0M0sRT2smiRN9TnHGsb1c1+VaLzUxoJMf1ouPu6503QCMQBA8T\n\tSScDbFtRbdVxpz5q9/3Ovi67vZraIwJGhtkSvk6X4wAi4VOLMUI5CsbxhYkuTMJ3Ke\n\tmUyZCyhUMlBNWPTiYY1L9pWJjis1gVnekl5nIb0uyTp+9kQeWDp6NCnLSpAjWHbLDv\n\tgF6sUkS4ZfAtQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1653940718;\n\tbh=72jAKo3eeTyauBsQLTbTw6gLTJdTXFDuI9CtEAdKsQQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=rgOXnDgebE68idAVmxAg35ATj+nKa4rlF1C22TWVYt81WvzVl9/fnfLakEJLJ3DiK\n\tvwtVNkhHgH/xuGRv1K5fBtQSrt72x2tg7mnuVtjDXrbyGAvfUbPYwOF6VO44ORnGyA\n\tnt7KVuA4QnUBfzgf+YtlWGfnixl1hC9w9b18DVzU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rgOXnDge\"; dkim-atps=neutral","Message-ID":"<528c3ba3-18ce-be13-27c4-22d8ff04de5a@ideasonboard.com>","Date":"Mon, 30 May 2022 21:58:34 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20220527093440.953377-1-paul.elder@ideasonboard.com>\n\t<20220527093440.953377-4-paul.elder@ideasonboard.com>\n\t<20220530095655.mlagqbi2qcae6zfn@uno.localdomain>\n\t<9eb7f714-8d9d-59e1-2924-02d5e253b521@ideasonboard.com>\n\t<20220530142705.72tsrmn63hcw7xqr@uno.localdomain>","In-Reply-To":"<20220530142705.72tsrmn63hcw7xqr@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23285,"web_url":"https://patchwork.libcamera.org/comment/23285/","msgid":"","date":"2022-06-02T07:32:13","subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nJust to re-iterate on point,\n\nOn 5/30/22 21:58, Umang Jain via libcamera-devel wrote:\n> Hi Jacopo,\n>\n> On 5/30/22 16:27, Jacopo Mondi wrote:\n>> Hi Umang,\n>>\n>> On Mon, May 30, 2022 at 03:41:20PM +0200, Umang Jain wrote:\n>>> Hi Jacopo,\n>>>\n>>> On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote:\n>>>> Hi Paul\n>>>>\n>>>> On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote:\n>>>>> From: Jacopo Mondi \n>>>>>\n>>>>> Mapped streams are generated by post-processing and always require a\n>>>>> source buffer from where to process image data from.\n>>>>>\n>>>>> In case a Mapped stream is requested but its source stream is not, it\n>>>>> is required to allocate a buffer on the fly and add it to the\n>>>>> libcamera::Request.\n>>>>>\n>>>>> Make sure a source stream is available for all mapped streams, and if\n>>>>> that's not the case, add a dedicated buffer to the request for that\n>>>>> purpose.\n>>>>>\n>>>>> Signed-off-by: Jacopo Mondi \n>>>>> Signed-off-by: Paul Elder \n>>>>>\n>>>>> ---\n>>>>> Changes in v2:\n>>>>> - cosmetic changes in code\n>>>>> - fix typo in the commit message\n>>>>> ---\n>>>>>    src/android/camera_device.cpp | 80 \n>>>>> +++++++++++++++++++++++++++++++----\n>>>>>    1 file changed, 72 insertions(+), 8 deletions(-)\n>>>>>\n>>>>> diff --git a/src/android/camera_device.cpp \n>>>>> b/src/android/camera_device.cpp\n>>>>> index 20599665..95c14f60 100644\n>>>>> --- a/src/android/camera_device.cpp\n>>>>> +++ b/src/android/camera_device.cpp\n>>>>> @@ -9,6 +9,7 @@\n>>>>>\n>>>>>    #include \n>>>>>    #include \n>>>>> +#include \n>>>>>    #include \n>>>>>    #include \n>>>>>    #include \n>>>>> @@ -923,6 +924,32 @@ int \n>>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>>> *camera3Reques\n>>>>>        LOG(HAL, Debug) << \"Queueing request \" << \n>>>>> descriptor->request_->cookie()\n>>>>>                << \" with \" << descriptor->buffers_.size() << \" \n>>>>> streams\";\n>>>>>\n>>>>> +    /*\n>>>>> +     * Collect the CameraStream associated to each requested \n>>>>> capture stream.\n>>>>> +     * Since requestedStreams is an std:set<>, no duplications \n>>>>> can happen.\n>>>>                                           std::set<>\n>>>>\n>>>>> +     */\n>>>>> +    std::set requestedStreams;\n>>>>> +    for (const auto &[i, buffer] : \n>>>>> utils::enumerate(descriptor->buffers_)) {\n>>>>> +        CameraStream *cameraStream = buffer.stream;\n>>>>> +\n>>>>> +        switch (cameraStream->type()) {\n>>>>> +        case CameraStream::Type::Mapped:\n>>>>> + requestedStreams.insert(cameraStream->sourceStream());\n>>>\n>>> Can sourceStream field for Mapped streams be nullptr here? Should we \n>>> ensure\n>>> it via an ASSERT?\n>\n>\n> Would like your thoughts on this as well..\n\n\nProbably we should merge in with the ASSERT(cameraStream->sourceStream() \n!= nullptr) as a safety check.\nThat gives me a bit of mental peace:\n\nWith that,\n\nReviewed-by: Umang Jain \n\n>\n>>>\n>>>>> +            break;\n>>>>> +\n>>>>> +        case CameraStream::Type::Direct:\n>>>>> +        case CameraStream::Type::Internal:\n>>>>> +            requestedStreams.insert(cameraStream);\n>>>>> +            break;\n>>>>> +        }\n>>>>> +    }\n>>>>> +\n>>>>> +    /*\n>>>>> +     * Process all the Direct and Internal streams, for which the \n>>>>> CameraStream\n>>>>> +     * they refer to is the one that points to the right \n>>>>> libcamera::Stream.\n>>>>> +     *\n>>>>> +     * Streams of type Mapped will be handled later.\n>>>>> +     */\n>>>>>        for (const auto &[i, buffer] : \n>>>>> utils::enumerate(descriptor->buffers_)) {\n>>>>>            CameraStream *cameraStream = buffer.stream;\n>>>>>            camera3_stream_t *camera3Stream = \n>>>>> cameraStream->camera3Stream();\n>>>>> @@ -945,14 +972,6 @@ int \n>>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>>> *camera3Reques\n>>>>>\n>>>>>            switch (cameraStream->type()) {\n>>>>>            case CameraStream::Type::Mapped:\n>>>>> -            /*\n>>>>> -             * Mapped streams don't need buffers added to the\n>>>>> -             * Request.\n>>>>> -             */\n>>>>> -            LOG(HAL, Debug) << ss.str() << \" (mapped)\";\n>>>>> -\n>>>>> - descriptor->pendingStreamsToProcess_.insert(\n>>>>> -                { cameraStream, &buffer });\n>>>>>                continue;\n>>>>>\n>>>>>            case CameraStream::Type::Direct:\n>>>>> @@ -996,6 +1015,51 @@ int \n>>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t \n>>>>> *camera3Reques\n>>>>>            auto fence = \n>>>>> std::make_unique(std::move(acquireFence));\n>>>>> descriptor->request_->addBuffer(cameraStream->stream(),\n>>>>>                            frameBuffer, std::move(fence));\n>>>>> +\n>>>>> +        requestedStreams.erase(cameraStream);\n>>>>> +    }\n>>>>> +\n>>>>> +    /*\n>>>>> +     * Now handle the Mapped streams. If no buffer has been added \n>>>>> for them\n>>>>> +     * as their corresponding direct source stream has not been \n>>>>> requested,\n>>>>> +     * add it here.\n>>>\n>>> I am wondering a situation where a direct stream D, and a mapped \n>>> stream M,\n>>> is requested and M is mapped onto D so,\n>>>\n>>>          M->sourceStream = D;\n>>>\n>>> Provided the requestedStreams is a std::set<> where no duplications can\n>>> happen, and given the above scenario:\n>>> I see the requestedStreams will consist of  { D } while populating \n>>> the set\n>>> in the switch-case above, which then  gets\n>>> erased from the requestedStreams (above the comment block) so now, the\n>>> requestedStreams become an empty set { } here . . .\n>>>\n>>>>> +     */\n>>>>> +    for (const auto &[i, buffer] : \n>>>>> utils::enumerate(descriptor->buffers_)) {\n>>>>> +        CameraStream *cameraStream = buffer.stream;\n>>>>> +        camera3_stream_t *camera3Stream = \n>>>>> cameraStream->camera3Stream();\n>>>>> +\n>>>>> +        if (cameraStream->type() != CameraStream::Type::Mapped)\n>>>>> +            continue;\n>>>>> +\n>>>>> +        LOG(HAL, Debug) << i << \" - (\" << camera3Stream->width << \n>>>>> \"x\"\n>>>>> +                << camera3Stream->height << \")\"\n>>>>> +                << \"[\" << utils::hex(camera3Stream->format) << \"] \n>>>>> -> \"\n>>>>> +                << \"(\" << \n>>>>> cameraStream->configuration().size.toString() << \")[\"\n>>>>> +                << \n>>>>> cameraStream->configuration().pixelFormat.toString() << \"]\"\n>>>>> +                << \" (mapped)\";\n>>>>> +\n>>>>> +        MutexLocker lock(descriptor->streamsProcessMutex_);\n>>>>> +        descriptor->pendingStreamsToProcess_.insert({ \n>>>>> cameraStream, &buffer });\n>>>>> +\n>>>>> +        /*\n>>>>> +         * Make sure the CameraStream this stream is mapped on \n>>>>> has been\n>>>>> +         * added to the request.\n>>>>> +         */\n>>>>> +        CameraStream *sourceStream = cameraStream->sourceStream();\n>>>>> +        if (requestedStreams.find(sourceStream) == \n>>>>> requestedStreams.end())\n>>>\n>>> and then while handling mapped streams, it will try to find { D }  in\n>>> requestedStreams (which is now empty)\n>>>\n>>>>> +            continue;\n>> So that the find() operation returns == requestedStreams.end() and we\n>\n>\n> Ah .. so silly of me thinking the comparator as != requestedStreams.end()\n> instead of == requestedStreams.end(). So code patterns do stick in the \n> brain :D\n>\n>> continue here\n>>\n>>>>> +\n>>>>> +        /*\n>>>>> +         * If that's not the case, we need to add a buffer to the \n>>>>> request\n>>>>> +         * for this stream.\n>>>>> +         */\n>>>>> +        FrameBuffer *frameBuffer = cameraStream->getBuffer();\n>>>>> +        buffer.internalBuffer = frameBuffer;\n>>>>> +\n>>>>> + descriptor->request_->addBuffer(sourceStream->stream(),\n>>>>> +                        frameBuffer, nullptr);\n>>>\n>>> ... and add a internal buffer for D\n>>>\n>>> so we have 2 buffers for D in the end ?\n>> So we don't get here and won't add the buffer twice.\n>\n>\n> Makes sense now\n>\n>>\n>> Have I missed something on you comment ?\n>\n>\n> No. It looks good to me now!\n>\n>>\n>>>>> +\n>>>>> +        requestedStreams.erase(sourceStream);\n>>>>>        }\n>>>> We could possibily make sure here that now requestedStreams is empty,\n>>>> but that's just an additional safety check.\n>>>>\n>>>> The patch still looks ok to me.\n>>>>\n>>>>>        /*\n>>>>> -- \n>>>>> 2.30.2\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 C0E9DBD160\n\tfor ;\n\tThu, 2 Jun 2022 07:32:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1401965637;\n\tThu, 2 Jun 2022 09:32:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1BF066040E\n\tfor ;\n\tThu, 2 Jun 2022 09:32:17 +0200 (CEST)","from [192.168.0.71] (static-127-186-62-95.ipcom.comunitel.net\n\t[95.62.186.127])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 783956BD;\n\tThu, 2 Jun 2022 09:32:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654155138;\n\tbh=rfQRSP8qmmT7mGHa1DpPrtoikQOFpBtV312DKI/P0Fs=;\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=0JhdqtvezIRjtgdqsrslMG5mvGXiykbJCJaADX6o1jnZT21XNUqQNvUTqno7lxCQ7\n\tWovnnX5TxVvyvJRxf1w3YoVH2YUxPd1mZMl00DqjjueQ8HwcrVk4SNEp22qvlJsNaf\n\t2xcuSTyFy+3+1PJ96erRLWzVEiHD6Zc6wZBwnTNPyRDXcv/vaB3FBgfef7Q9WjqNkf\n\t+bDGBoY6fldYZWkVbMLq2Na+BNFdCjQMHwxNdO2nTI3wBmFphuwb8tCCtjkEeR2luJ\n\tMz5I1/n/kKDu28vc4s08lNbAvsdheuFj4P2h1MAoyIBqdmHv6/4QpDJUF6tbKzYqRz\n\tyNFGwhIyNAg2A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654155136;\n\tbh=rfQRSP8qmmT7mGHa1DpPrtoikQOFpBtV312DKI/P0Fs=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Jh3G4t4yHK2yLsJS+wiVPAqhOX2RzYbrb/uoXQF2JbpGCixSzhJ01Vo4ehOX2ISZC\n\taIa9ZDvK7qmRPsyeD6StAhukJN3YLFpj3fl8ItNrO7c8JuZF/7xIDW2LxJ2URYCeTX\n\t3h0bL0SNtS5rrYLkSs4OBS24j6c9dGQwxE5eRV68="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Jh3G4t4y\"; dkim-atps=neutral","Message-ID":"","Date":"Thu, 2 Jun 2022 09:32:13 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20220527093440.953377-1-paul.elder@ideasonboard.com>\n\t<20220527093440.953377-4-paul.elder@ideasonboard.com>\n\t<20220530095655.mlagqbi2qcae6zfn@uno.localdomain>\n\t<9eb7f714-8d9d-59e1-2924-02d5e253b521@ideasonboard.com>\n\t<20220530142705.72tsrmn63hcw7xqr@uno.localdomain>\n\t<528c3ba3-18ce-be13-27c4-22d8ff04de5a@ideasonboard.com>","In-Reply-To":"<528c3ba3-18ce-be13-27c4-22d8ff04de5a@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23290,"web_url":"https://patchwork.libcamera.org/comment/23290/","msgid":"","date":"2022-06-02T08:35:30","subject":"Re: [libcamera-devel] [PATCH v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jun 02, 2022 at 09:32:13AM +0200, Umang Jain via libcamera-devel wrote:\n> On 5/30/22 21:58, Umang Jain via libcamera-devel wrote:\n> > On 5/30/22 16:27, Jacopo Mondi wrote:\n> >> On Mon, May 30, 2022 at 03:41:20PM +0200, Umang Jain wrote:\n> >>> On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote:\n> >>>> On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote:\n> >>>>> From: Jacopo Mondi \n> >>>>>\n> >>>>> Mapped streams are generated by post-processing and always require a\n> >>>>> source buffer from where to process image data from.\n\ns/from where //\n\n> >>>>>\n> >>>>> In case a Mapped stream is requested but its source stream is not, it\n> >>>>> is required to allocate a buffer on the fly and add it to the\n> >>>>> libcamera::Request.\n> >>>>>\n> >>>>> Make sure a source stream is available for all mapped streams, and if\n> >>>>> that's not the case, add a dedicated buffer to the request for that\n> >>>>> purpose.\n> >>>>>\n> >>>>> Signed-off-by: Jacopo Mondi \n> >>>>> Signed-off-by: Paul Elder \n> >>>>>\n> >>>>> ---\n> >>>>> Changes in v2:\n> >>>>> - cosmetic changes in code\n> >>>>> - fix typo in the commit message\n> >>>>> ---\n> >>>>>    src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++----\n> >>>>>    1 file changed, 72 insertions(+), 8 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/android/camera_device.cpp \n> >>>>> b/src/android/camera_device.cpp\n> >>>>> index 20599665..95c14f60 100644\n> >>>>> --- a/src/android/camera_device.cpp\n> >>>>> +++ b/src/android/camera_device.cpp\n> >>>>> @@ -9,6 +9,7 @@\n> >>>>>\n> >>>>>  #include \n> >>>>>  #include \n> >>>>> +#include \n> >>>>>  #include \n> >>>>>  #include \n> >>>>>  #include \n> >>>>> @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>>>        LOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n> >>>>>                << \" with \" << descriptor->buffers_.size() << \" streams\";\n> >>>>>\n> >>>>> +    /*\n> >>>>> +     * Collect the CameraStream associated to each requested capture stream.\n> >>>>> +     * Since requestedStreams is an std:set<>, no duplications can happen.\n> >>>>                                           std::set<>\n> >>>>\n> >>>>> +     */\n> >>>>> +    std::set requestedStreams;\n> >>>>> +    for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> >>>>> +        CameraStream *cameraStream = buffer.stream;\n> >>>>> +\n> >>>>> +        switch (cameraStream->type()) {\n> >>>>> +        case CameraStream::Type::Mapped:\n> >>>>> + requestedStreams.insert(cameraStream->sourceStream());\n> >>>\n> >>> Can sourceStream field for Mapped streams be nullptr here? Should we ensure\n> >>> it via an ASSERT?\n> >\n> > Would like your thoughts on this as well..\n> \n> Probably we should merge in with the ASSERT(cameraStream->sourceStream() \n> != nullptr) as a safety check.\n> That gives me a bit of mental peace:\n\nIt shouldn't be null, so an assert is enough if it helps you sleeping\n:-)\n\n> With that,\n> \n> Reviewed-by: Umang Jain \n> \n> >>>>> +            break;\n> >>>>> +\n> >>>>> +        case CameraStream::Type::Direct:\n> >>>>> +        case CameraStream::Type::Internal:\n> >>>>> +            requestedStreams.insert(cameraStream);\n> >>>>> +            break;\n> >>>>> +        }\n> >>>>> +    }\n> >>>>> +\n> >>>>> +    /*\n> >>>>> +     * Process all the Direct and Internal streams, for which the CameraStream\n> >>>>> +     * they refer to is the one that points to the right libcamera::Stream.\n\nI have trouble parsing this, what is \"the right libcamera::Stream\" ? Do\nyou mean something along the lines of\n\n\t/*\n\t * Process all the Direct and Internal streams first, they map directly\n\t * to a libcamera stream.\n\t */\n\n> >>>>> +     *\n> >>>>> +     * Streams of type Mapped will be handled later.\n> >>>>> +     */\n> >>>>>        for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> >>>>>            CameraStream *cameraStream = buffer.stream;\n> >>>>>            camera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> >>>>> @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>>>\n> >>>>>            switch (cameraStream->type()) {\n> >>>>>            case CameraStream::Type::Mapped:\n> >>>>> -            /*\n> >>>>> -             * Mapped streams don't need buffers added to the\n> >>>>> -             * Request.\n> >>>>> -             */\n> >>>>> -            LOG(HAL, Debug) << ss.str() << \" (mapped)\";\n> >>>>> -\n> >>>>> - descriptor->pendingStreamsToProcess_.insert(\n> >>>>> -                { cameraStream, &buffer });\n\nLet's keep a comment:\n\n\t\t\t/* Mapped streams will be handled in the next loop. */\n\n> >>>>>                continue;\n> >>>>>\n> >>>>>            case CameraStream::Type::Direct:\n> >>>>> @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>>>>            auto fence = std::make_unique(std::move(acquireFence));\n> >>>>> descriptor->request_->addBuffer(cameraStream->stream(),\n> >>>>>                            frameBuffer, std::move(fence));\n> >>>>> +\n> >>>>> +        requestedStreams.erase(cameraStream);\n> >>>>> +    }\n> >>>>> +\n> >>>>> +    /*\n> >>>>> +     * Now handle the Mapped streams. If no buffer has been added for them\n> >>>>> +     * as their corresponding direct source stream has not been requested,\n> >>>>> +     * add it here.\n\n\t* Now handle the Mapped streams. If no buffer has been added for them,\n\t* because their corresponding direct source stream is not part of this\n\t* particular request, add one here.\n\n\n> >>>\n> >>> I am wondering a situation where a direct stream D, and a mapped stream M,\n> >>> is requested and M is mapped onto D so,\n> >>>\n> >>>          M->sourceStream = D;\n> >>>\n> >>> Provided the requestedStreams is a std::set<> where no duplications can\n> >>> happen, and given the above scenario:\n> >>> I see the requestedStreams will consist of  { D } while populating the set\n> >>> in the switch-case above, which then  gets\n> >>> erased from the requestedStreams (above the comment block) so now, the\n> >>> requestedStreams become an empty set { } here . . .\n> >>>\n> >>>>> +     */\n> >>>>> +    for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) {\n> >>>>> +        CameraStream *cameraStream = buffer.stream;\n> >>>>> +        camera3_stream_t *camera3Stream = cameraStream->camera3Stream();\n> >>>>> +\n> >>>>> +        if (cameraStream->type() != CameraStream::Type::Mapped)\n> >>>>> +            continue;\n> >>>>> +\n> >>>>> +        LOG(HAL, Debug) << i << \" - (\" << camera3Stream->width << \"x\"\n> >>>>> +                << camera3Stream->height << \")\"\n> >>>>> +                << \"[\" << utils::hex(camera3Stream->format) << \"] -> \"\n> >>>>> +                << \"(\" << cameraStream->configuration().size.toString() << \")[\"\n> >>>>> +                << cameraStream->configuration().pixelFormat.toString() << \"]\"\n\nWe can now drop the .toString() :-)\n\n> >>>>> +                << \" (mapped)\";\n> >>>>> +\n> >>>>> +        MutexLocker lock(descriptor->streamsProcessMutex_);\n> >>>>> +        descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });\n> >>>>> +\n> >>>>> +        /*\n> >>>>> +         * Make sure the CameraStream this stream is mapped on has been\n\ns/mapped on/mapped to/\n\n> >>>>> +         * added to the request.\n> >>>>> +         */\n> >>>>> +        CameraStream *sourceStream = cameraStream->sourceStream();\n> >>>>> +        if (requestedStreams.find(sourceStream) == requestedStreams.end())\n> >>>\n> >>> and then while handling mapped streams, it will try to find { D }  in\n> >>> requestedStreams (which is now empty)\n> >>>\n> >>>>> +            continue;\n> >>\n> >> So that the find() operation returns == requestedStreams.end() and we\n> >\n> > Ah .. so silly of me thinking the comparator as != requestedStreams.end()\n> > instead of == requestedStreams.end(). So code patterns do stick in the \n> > brain :D\n\nPart of the confusion may come from the variable name. It contains the\nstreams that haven't been added to the request yet. How about renaming\nit to unhandledStreams ?\n\nThinking more about it, couldn't we simplify the code by inverting the\nlogic ? It seems we could drop the first loop that populates\nrequestedStreams, and instead add streams to requestedStreams in the\nloop that handles the Direct and Internal streams. The check here would\nthen become != requestedStreams.end(), and below the erase() would be\nturned into an insert().\n\nReviewed-by: Laurent Pinchart \n\n> >\n> >> continue here\n> >>\n> >>>>> +\n> >>>>> +        /*\n> >>>>> +         * If that's not the case, we need to add a buffer to the request\n> >>>>> +         * for this stream.\n> >>>>> +         */\n> >>>>> +        FrameBuffer *frameBuffer = cameraStream->getBuffer();\n> >>>>> +        buffer.internalBuffer = frameBuffer;\n> >>>>> +\n> >>>>> + descriptor->request_->addBuffer(sourceStream->stream(),\n> >>>>> +                        frameBuffer, nullptr);\n> >>>\n> >>> ... and add a internal buffer for D\n> >>>\n> >>> so we have 2 buffers for D in the end ?\n> >>\n> >> So we don't get here and won't add the buffer twice.\n> >\n> > Makes sense now\n> >\n> >> Have I missed something on you comment ?\n> >\n> > No. It looks good to me now!\n> >\n> >>>>> +\n> >>>>> +        requestedStreams.erase(sourceStream);\n> >>>>>        }\n> >>>>\n> >>>> We could possibily make sure here that now requestedStreams is empty,\n> >>>> but that's just an additional safety check.\n> >>>>\n> >>>> The patch still looks ok to me.\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 3536CBD161\n\tfor ;\n\tThu, 2 Jun 2022 08:35:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F7D565642;\n\tThu, 2 Jun 2022 10:35:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2428065636\n\tfor ;\n\tThu, 2 Jun 2022 10:35:35 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(lmontsouris-659-1-41-236.w92-154.abo.wanadoo.fr [92.154.76.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9C5EC6BD;\n\tThu, 2 Jun 2022 10:35:34 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654158936;\n\tbh=VQB+2ZKJB6V3SXGE2hFfDmy/L/xhobx/1tJ1G00mZow=;\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=L2LNtYXajsg07oect7FmIHsizQ1HZBy0bcau1TaN/hdHDZk2uaOic8rRkJeNXlOJA\n\t7oTD72ceGNkhz2rt+wO9c4dnzYcYDonca3Z50kuyblQMg0tP4TZYdTCObwv4xDFdPh\n\tO3IuG+tSGIm/NU/XZ1OjVP+XJI8+U0yXwKzNkEC9P9P2g8j6iBqqMXNDDznO3XHk+/\n\tYXvlIMg57VvtZ85UvQVzm6jNrBXIv9pR5e4brs6DlJncBtxVPGYlofpJiibHZtoEsw\n\t+sJya8egSn6g89W+0A1R2PuP0oU5K2lzfgc34g5pXyfy9ypgkYx5NLlEf/o0IoqNrm\n\te8w7wXEEJMXDw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654158934;\n\tbh=VQB+2ZKJB6V3SXGE2hFfDmy/L/xhobx/1tJ1G00mZow=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UsM44V2GHojATZ8+ic2c+6oo8fTsoeZJbOKpD8qGuBboO8EY06mpgnVsox1MfOIbW\n\tBxNZ/o272rniH2hkEKDXujxwqR4FhtsiS+LUFpzmbARY06RC5P8zbVUCz5TEshUMrw\n\tboM8EhXin9vkc/n0x7Nk6l5ljbRoYthb7zWhclbE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UsM44V2G\"; dkim-atps=neutral","Date":"Thu, 2 Jun 2022 11:35:30 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220527093440.953377-1-paul.elder@ideasonboard.com>\n\t<20220527093440.953377-4-paul.elder@ideasonboard.com>\n\t<20220530095655.mlagqbi2qcae6zfn@uno.localdomain>\n\t<9eb7f714-8d9d-59e1-2924-02d5e253b521@ideasonboard.com>\n\t<20220530142705.72tsrmn63hcw7xqr@uno.localdomain>\n\t<528c3ba3-18ce-be13-27c4-22d8ff04de5a@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 v2 3/5] android: camera_device:\n\tPostpone mapped streams handling","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\" "}}]