[{"id":34356,"web_url":"https://patchwork.libcamera.org/comment/34356/","msgid":"","date":"2025-05-27T07:27:13","subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 05. 26. 23:42 keltezéssel, Stefan Klug írta:\n> The waiting requests of one camera should not be able to influence\n> queuing to another camera. Therefore change the single waitingRequests_\n> queue into one queue per camera.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> include/libcamera/internal/camera.h | 2 ++\n> include/libcamera/internal/pipeline_handler.h | 3 --\n> src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------\n> 3 files changed, 25 insertions(+), 14 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index 18f5c32a18e4..8a2e9ed5894d 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -36,6 +37,7 @@ public:\n> \tconst PipelineHandler *pipe() const { return pipe_.get(); }\n> \n> \tstd::list queuedRequests_;\n> +\tstd::queue waitingRequests_;\n> \tControlInfoMap controlInfo_;\n> \tControlList properties_;\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 972a2fa65310..dedc29e815fb 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -8,7 +8,6 @@\n> #pragma once\n> \n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -94,8 +93,6 @@ private:\n> \tstd::vector> mediaDevices_;\n> \tstd::vector> cameras_;\n> \n> -\tstd::queue waitingRequests_;\n> -\n> \tconst char *name_;\n> \tunsigned int useCount_;\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index d84dff3c9f19..14d8602e67d8 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera)\n> \t/* Stop the pipeline handler and let the queued requests complete. */\n> \tstopDevice(camera);\n> \n> +\tCamera::Private *data = camera->_d();\n> +\n> \t/* Cancel and signal as complete all waiting requests. */\n> -\twhile (!waitingRequests_.empty()) {\n> -\t\tRequest *request = waitingRequests_.front();\n> -\t\twaitingRequests_.pop();\n> +\twhile (!data->waitingRequests_.empty()) {\n> +\t\tRequest *request = data->waitingRequests_.front();\n> +\t\tdata->waitingRequests_.pop();\n> \t\tcancelRequest(request);\n> \t}\n> \n> \t/* Make sure no requests are pending. */\n> -\tCamera::Private *data = camera->_d();\n> \tASSERT(data->queuedRequests_.empty());\n> \n> \tdata->requestSequence_ = 0;\n> @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request)\n> {\n> \tLIBCAMERA_TRACEPOINT(request_queue, request);\n> \n> -\twaitingRequests_.push(request);\n> +\tCamera *camera = request->_d()->camera();\n> +\tCamera::Private *data = camera->_d();\n> +\tdata->waitingRequests_.push(request);\n> \n> \trequest->_d()->prepare(300ms);\n> }\n> @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request)\n> */\n> void PipelineHandler::doQueueRequests()\n> {\n> -\twhile (!waitingRequests_.empty()) {\n> -\t\tRequest *request = waitingRequests_.front();\n> -\t\tif (!request->_d()->prepared_)\n> -\t\t\tbreak;\n> +\tfor (const std::weak_ptr &ptr : cameras_) {\n> +\t\tstd::shared_ptr camera = ptr.lock();\n> +\t\tif (!camera)\n> +\t\t\tcontinue;\n> \n> -\t\tdoQueueRequest(request);\n> -\t\twaitingRequests_.pop();\n> +\t\tCamera::Private *data = camera->_d();\n> +\t\twhile (!data->waitingRequests_.empty()) {\n\nAs far as I understand `doQueueRequests()` runs when a request becomes \"prepared\".\nSo I would expect the above inner loop to be sufficient. Is there any reason why\nthe waiting request queues of all cameras are processed?\n\n\n> +\t\t\tRequest *request = data->waitingRequests_.front();\n> +\t\t\tif (!request->_d()->prepared_)\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tdoQueueRequest(request);\n> +\t\t\tdata->waitingRequests_.pop();\n> +\t\t}\n> \t}\n> }\n> \n> @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request)\n> \t\tdata->queuedRequests_.pop_front();\n> \t\tcamera->requestComplete(req);\n> \t}\n> +\n> +\tdoQueueRequests();\n\nThis only purpose of this is to handle early stopping due to `maxQueuedRequestsDevice()`, right?\nIf so, then I think this should be in the next patch.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> }\n> \n> /**\n> --\n> 2.43.0\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 E3520C31E9\n\tfor ;\n\tTue, 27 May 2025 07:27:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A2BC68D9E;\n\tTue, 27 May 2025 09:27:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 920DA68D8B\n\tfor ;\n\tTue, 27 May 2025 09:27:24 +0200 (CEST)","from [192.168.33.12] (185.221.141.78.nat.pool.zt.hu\n\t[185.221.141.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 82418725;\n\tTue, 27 May 2025 09:26:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"BdiSYeM4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1748330812;\n\tbh=7aHj/4cgLJQWNabYIeeCu4Z5hFt/UosOc+s2kGxFyBo=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=BdiSYeM4y1cq1NUJuxWS5TnvxvQkfPcZSwy84p4bjjJp2Bf/Gmzj5oaWasO+WFuIM\n\tjbRORqVwUo9cPuGurU3S/nSUe+Ljo1GHwwZQbhuRjDllWgcWuND1Jz0Q9vfHcR8gDA\n\tuSi6qi/FJ6usSaE1AaanHTbBd+7LfLuLjYv+NmME=","Message-ID":"","Date":"Tue, 27 May 2025 09:27:13 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250526214224.13631-1-stefan.klug@ideasonboard.com>\n\t\n\t<20250526214224.13631-2-stefan.klug@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250526214224.13631-2-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34357,"web_url":"https://patchwork.libcamera.org/comment/34357/","msgid":"<0eaeb442-d34c-4d8a-b985-ac297e87e45c@pengutronix.de>","date":"2025-05-27T09:06:31","subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","submitter":{"id":225,"url":"https://patchwork.libcamera.org/api/people/225/","name":"Sven Püschel","email":"s.pueschel@pengutronix.de"},"content":"Hi Barnabás,\n\nOn 5/27/25 09:27, Barnabás Pőcze wrote:\n> Hi\n>\n> 2025. 05. 26. 23:42 keltezéssel, Stefan Klug írta:\n>> The waiting requests of one camera should not be able to influence\n>> queuing to another camera. Therefore change the single waitingRequests_\n>> queue into one queue per camera.\n>>\n>> Signed-off-by: Stefan Klug \n>> ---\n>>   include/libcamera/internal/camera.h           |  2 ++\n>>   include/libcamera/internal/pipeline_handler.h |  3 --\n>>   src/libcamera/pipeline_handler.cpp            | 34 +++++++++++++------\n>>   3 files changed, 25 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/camera.h \n>> b/include/libcamera/internal/camera.h\n>> index 18f5c32a18e4..8a2e9ed5894d 100644\n>> --- a/include/libcamera/internal/camera.h\n>> +++ b/include/libcamera/internal/camera.h\n>> @@ -10,6 +10,7 @@\n>>   #include \n>>   #include \n>>   #include \n>> +#include \n>>   #include \n>>   #include \n>>   #include \n>> @@ -36,6 +37,7 @@ public:\n>>       const PipelineHandler *pipe() const { return pipe_.get(); }\n>>\n>>       std::list queuedRequests_;\n>> +    std::queue waitingRequests_;\n>>       ControlInfoMap controlInfo_;\n>>       ControlList properties_;\n>>\n>> diff --git a/include/libcamera/internal/pipeline_handler.h \n>> b/include/libcamera/internal/pipeline_handler.h\n>> index 972a2fa65310..dedc29e815fb 100644\n>> --- a/include/libcamera/internal/pipeline_handler.h\n>> +++ b/include/libcamera/internal/pipeline_handler.h\n>> @@ -8,7 +8,6 @@\n>>   #pragma once\n>>\n>>   #include \n>> -#include \n>>   #include \n>>   #include \n>>   #include \n>> @@ -94,8 +93,6 @@ private:\n>>       std::vector> mediaDevices_;\n>>       std::vector> cameras_;\n>>\n>> -    std::queue waitingRequests_;\n>> -\n>>       const char *name_;\n>>       unsigned int useCount_;\n>>\n>> diff --git a/src/libcamera/pipeline_handler.cpp \n>> b/src/libcamera/pipeline_handler.cpp\n>> index d84dff3c9f19..14d8602e67d8 100644\n>> --- a/src/libcamera/pipeline_handler.cpp\n>> +++ b/src/libcamera/pipeline_handler.cpp\n>> @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera)\n>>       /* Stop the pipeline handler and let the queued requests \n>> complete. */\n>>       stopDevice(camera);\n>>\n>> +    Camera::Private *data = camera->_d();\n>> +\n>>       /* Cancel and signal as complete all waiting requests. */\n>> -    while (!waitingRequests_.empty()) {\n>> -        Request *request = waitingRequests_.front();\n>> -        waitingRequests_.pop();\n>> +    while (!data->waitingRequests_.empty()) {\n>> +        Request *request = data->waitingRequests_.front();\n>> +        data->waitingRequests_.pop();\n>>           cancelRequest(request);\n>>       }\n>>\n>>       /* Make sure no requests are pending. */\n>> -    Camera::Private *data = camera->_d();\n>>       ASSERT(data->queuedRequests_.empty());\n>>\n>>       data->requestSequence_ = 0;\n>> @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request)\n>>   {\n>>       LIBCAMERA_TRACEPOINT(request_queue, request);\n>>\n>> -    waitingRequests_.push(request);\n>> +    Camera *camera = request->_d()->camera();\n>> +    Camera::Private *data = camera->_d();\n>> +    data->waitingRequests_.push(request);\n>>\n>>       request->_d()->prepare(300ms);\n>>   }\n>> @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request \n>> *request)\n>>    */\n>>   void PipelineHandler::doQueueRequests()\n>>   {\n>> -    while (!waitingRequests_.empty()) {\n>> -        Request *request = waitingRequests_.front();\n>> -        if (!request->_d()->prepared_)\n>> -            break;\n>> +    for (const std::weak_ptr &ptr : cameras_) {\n>> +        std::shared_ptr camera = ptr.lock();\n>> +        if (!camera)\n>> +            continue;\n>>\n>> -        doQueueRequest(request);\n>> -        waitingRequests_.pop();\n>> +        Camera::Private *data = camera->_d();\n>> +        while (!data->waitingRequests_.empty()) {\n>\n> As far as I understand `doQueueRequests()` runs when a request becomes \n> \"prepared\".\n> So I would expect the above inner loop to be sufficient. Is there any \n> reason why\n> the waiting request queues of all cameras are processed?\n\nThis results from the fact that the patchset splits the waitingRequests_ \nfrom the PipelineHandler to the (multiple) individual Camera classes. \nTherefore an additional outer loop is necessary to iterate though all \ncamera class instances holding potential waitingRequests_.\n\nI would propose to add a Camera shared pointer as an argument to the \ndoQueueRequests function (and adjust the signal) to avoid looping over \nall cameras.\n\n>\n>\n>> +            Request *request = data->waitingRequests_.front();\n>> +            if (!request->_d()->prepared_)\n>> +                break;\n>> +\n>> +            doQueueRequest(request);\n>> +            data->waitingRequests_.pop();\n>> +        }\n>>       }\n>>   }\n>>\n>> @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request \n>> *request)\n>>           data->queuedRequests_.pop_front();\n>>           camera->requestComplete(req);\n>>       }\n>> +\n>> +    doQueueRequests();\n>\n> This only purpose of this is to handle early stopping due to \n> `maxQueuedRequestsDevice()`, right?\n> If so, then I think this should be in the next patch.\n\nyeah, exactly. I agree to move it to the next patch.\n\n\nSincerely\n\n     Sven\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n>>   }\n>>\n>>   /**\n>> -- \n>> 2.43.0\n>>\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 322A4C3237\n\tfor ;\n\tTue, 27 May 2025 09:06:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E5DE868D9E;\n\tTue, 27 May 2025 11:06:32 +0200 (CEST)","from metis.whiteo.stw.pengutronix.de\n\t(metis.whiteo.stw.pengutronix.de [IPv6:2a0a:edc0:2:b01:1d::104])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0C2468D8B\n\tfor ;\n\tTue, 27 May 2025 11:06:31 +0200 (CEST)","from ptz.office.stw.pengutronix.de ([2a0a:edc0:0:900:1d::77]\n\thelo=[127.0.0.1])\n\tby metis.whiteo.stw.pengutronix.de with esmtp (Exim 4.92)\n\t(envelope-from ) id 1uJqGV-0001HE-5e\n\tfor libcamera-devel@lists.libcamera.org;\n\tTue, 27 May 2025 11:06:31 +0200"],"Message-ID":"<0eaeb442-d34c-4d8a-b985-ac297e87e45c@pengutronix.de>","Date":"Tue, 27 May 2025 11:06:31 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","To":"libcamera-devel@lists.libcamera.org","References":"<20250526214224.13631-1-stefan.klug@ideasonboard.com>\n\t\n\t<20250526214224.13631-2-stefan.klug@ideasonboard.com>\n\t","Content-Language":"en-US","From":"=?utf-8?q?Sven_P=C3=BCschel?= ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-SA-Exim-Connect-IP":"2a0a:edc0:0:900:1d::77","X-SA-Exim-Mail-From":"s.pueschel@pengutronix.de","X-SA-Exim-Scanned":"No (on metis.whiteo.stw.pengutronix.de);\n\tSAEximRunCond expanded to false","X-PTX-Original-Recipient":"libcamera-devel@lists.libcamera.org","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34360,"web_url":"https://patchwork.libcamera.org/comment/34360/","msgid":"<174835997541.433416.6410424646899968905@ping.linuxembedded.co.uk>","date":"2025-05-27T15:32:55","subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-05-26 22:42:15)\n> The waiting requests of one camera should not be able to influence\n> queuing to another camera. Therefore change the single waitingRequests_\n> queue into one queue per camera.\n> \n\nEeek.\n\n\n> Signed-off-by: Stefan Klug \n> ---\n> include/libcamera/internal/camera.h | 2 ++\n> include/libcamera/internal/pipeline_handler.h | 3 --\n> src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------\n> 3 files changed, 25 insertions(+), 14 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index 18f5c32a18e4..8a2e9ed5894d 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -36,6 +37,7 @@ public:\n> const PipelineHandler *pipe() const { return pipe_.get(); }\n> \n> std::list queuedRequests_;\n> + std::queue waitingRequests_;\n> ControlInfoMap controlInfo_;\n> ControlList properties_;\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 972a2fa65310..dedc29e815fb 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -8,7 +8,6 @@\n> #pragma once\n> \n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -94,8 +93,6 @@ private:\n> std::vector> mediaDevices_;\n> std::vector> cameras_;\n> \n> - std::queue waitingRequests_;\n> -\n> const char *name_;\n> unsigned int useCount_;\n> \n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index d84dff3c9f19..14d8602e67d8 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera)\n> /* Stop the pipeline handler and let the queued requests complete. */\n> stopDevice(camera);\n> \n> + Camera::Private *data = camera->_d();\n> +\n> /* Cancel and signal as complete all waiting requests. */\n> - while (!waitingRequests_.empty()) {\n\nYikes, that was plain wrong wasn't it - it would cancel another cameras\nqueued requests if two cameras were running in parallel from the same\npipeline (which may not be common, but could be possible).\n\nSo this definitely seems like a worth while change.\nPossibly even a fixes tag, but I won't dwell on that.\n\n\n> - Request *request = waitingRequests_.front();\n> - waitingRequests_.pop();\n> + while (!data->waitingRequests_.empty()) {\n> + Request *request = data->waitingRequests_.front();\n> + data->waitingRequests_.pop();\n> cancelRequest(request);\n> }\n> \n> /* Make sure no requests are pending. */\n> - Camera::Private *data = camera->_d();\n> ASSERT(data->queuedRequests_.empty());\n\nThere's so much operating on the Camera::Private *data here I wonder if\nthere should be a Camera::Private::Stop() ... but I'm not sure that\nwould be helpful - we probably want to keep the Camera::Private as a\nsimple structure.\n\n\n> \n> data->requestSequence_ = 0;\n> @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request)\n> {\n> LIBCAMERA_TRACEPOINT(request_queue, request);\n> \n> - waitingRequests_.push(request);\n> + Camera *camera = request->_d()->camera();\n> + Camera::Private *data = camera->_d();\n> + data->waitingRequests_.push(request);\n> \n> request->_d()->prepare(300ms);\n> }\n> @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request)\n> */\n> void PipelineHandler::doQueueRequests()\n> {\n> - while (!waitingRequests_.empty()) {\n> - Request *request = waitingRequests_.front();\n> - if (!request->_d()->prepared_)\n> - break;\n> + for (const std::weak_ptr &ptr : cameras_) {\n> + std::shared_ptr camera = ptr.lock();\n> + if (!camera)\n> + continue;\n> \n> - doQueueRequest(request);\n> - waitingRequests_.pop();\n> + Camera::Private *data = camera->_d();\n> + while (!data->waitingRequests_.empty()) {\n> + Request *request = data->waitingRequests_.front();\n> + if (!request->_d()->prepared_)\n> + break;\n> +\n> + doQueueRequest(request);\n> + data->waitingRequests_.pop();\n> + }\n> }\n> }\n> \n> @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request)\n> data->queuedRequests_.pop_front();\n> camera->requestComplete(req);\n> }\n> +\n\n\t/* Allow any waiting requests to be queued to the pipeline. */\n\n> + doQueueRequests();\n\nI think I saw a later reply also suggest but perhaps:\n\tdoQueueRequests(camera);\n\ncould simplify - so that PipelineHandler::doQueueRequests() doesn't\niterate over cameras which are not operating...\n\nAt least being explicit would help make sure we don't have bugs that\ncameras only wake up and continue because of an event on a different\ncamera ... ... unless that's desired behaviour?\n\n\n> }\n> \n> /**\n> -- \n> 2.43.0\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 0CB82C3237\n\tfor ;\n\tTue, 27 May 2025 15:33:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17B5668DA0;\n\tTue, 27 May 2025 17:33:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C28668D94\n\tfor ;\n\tTue, 27 May 2025 17:32:58 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B7D972B3;\n\tTue, 27 May 2025 17:32:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nOu6JMLO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1748359952;\n\tbh=Zn7btf1zY1DyhfFQg4HtBDSz8zqsXuAyJfTU2VAFapk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=nOu6JMLOHPAkGrisggJDY4J1YnnnFpaL5QKZ39TX77JSxSX7vi7qosRJkjjKqE6Bs\n\ttDC9kGRQKkfi+dpJoBt3w83pjAk7OII0EMYjOLMf6mb/hDnD0P+clsd+aB9cSiI8Dq\n\tDdQMXIBCiKFmSkbjHI2fYATqytqs6cAfdU6BKtQo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250526214224.13631-2-stefan.klug@ideasonboard.com>","References":"<20250526214224.13631-1-stefan.klug@ideasonboard.com>\n\t<20250526214224.13631-2-stefan.klug@ideasonboard.com>","Subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 27 May 2025 16:32:55 +0100","Message-ID":"<174835997541.433416.6410424646899968905@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34387,"web_url":"https://patchwork.libcamera.org/comment/34387/","msgid":"","date":"2025-05-30T08:59:55","subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Mon, May 26, 2025 at 11:42:15PM +0200, Stefan Klug wrote:\n> The waiting requests of one camera should not be able to influence\n> queuing to another camera. Therefore change the single waitingRequests_\n> queue into one queue per camera.\n>\n> Signed-off-by: Stefan Klug \n> ---\n> include/libcamera/internal/camera.h | 2 ++\n> include/libcamera/internal/pipeline_handler.h | 3 --\n> src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------\n> 3 files changed, 25 insertions(+), 14 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index 18f5c32a18e4..8a2e9ed5894d 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -10,6 +10,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -36,6 +37,7 @@ public:\n> \tconst PipelineHandler *pipe() const { return pipe_.get(); }\n>\n> \tstd::list queuedRequests_;\n> +\tstd::queue waitingRequests_;\n\nSo we keep the storage in Camera::Private but only access it from the\nPipelineHandler base class ?\n\nI understand operating on the waitingRequst_ queue in the Camera class\nmight not be desirable, as it's way easier to do that in the\nPipelineHandler as it runs in the library thread so we don't have to\nworry about concurrency.\n\nHowever I'm not sure this is a good pattern, maybe we can create a map\nin the PipelineHandler base class that associates a Camera to a queue\n?\n\n> \tControlInfoMap controlInfo_;\n> \tControlList properties_;\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 972a2fa65310..dedc29e815fb 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -8,7 +8,6 @@\n> #pragma once\n>\n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -94,8 +93,6 @@ private:\n> \tstd::vector> mediaDevices_;\n> \tstd::vector> cameras_;\n>\n> -\tstd::queue waitingRequests_;\n> -\n> \tconst char *name_;\n> \tunsigned int useCount_;\n>\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index d84dff3c9f19..14d8602e67d8 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera)\n> \t/* Stop the pipeline handler and let the queued requests complete. */\n> \tstopDevice(camera);\n>\n> +\tCamera::Private *data = camera->_d();\n> +\n> \t/* Cancel and signal as complete all waiting requests. */\n> -\twhile (!waitingRequests_.empty()) {\n> -\t\tRequest *request = waitingRequests_.front();\n> -\t\twaitingRequests_.pop();\n> +\twhile (!data->waitingRequests_.empty()) {\n> +\t\tRequest *request = data->waitingRequests_.front();\n> +\t\tdata->waitingRequests_.pop();\n> \t\tcancelRequest(request);\n> \t}\n>\n> \t/* Make sure no requests are pending. */\n> -\tCamera::Private *data = camera->_d();\n> \tASSERT(data->queuedRequests_.empty());\n>\n> \tdata->requestSequence_ = 0;\n> @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request)\n> {\n> \tLIBCAMERA_TRACEPOINT(request_queue, request);\n>\n> -\twaitingRequests_.push(request);\n> +\tCamera *camera = request->_d()->camera();\n> +\tCamera::Private *data = camera->_d();\n> +\tdata->waitingRequests_.push(request);\n>\n> \trequest->_d()->prepare(300ms);\n> }\n> @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request)\n> */\n> void PipelineHandler::doQueueRequests()\n\nI've seen suggestions of passing a Camera * to this function, it might\nremove one outer loop indeed. The function has two callers\n\nvoid PipelineHandler::completeRequest(Request *request)\n\nwhich indeed can get to Camera *\n\nand void Request::Private::emitPrepareCompleted()\n\nwhich can get to a Camera * as well\n\n> {\n> -\twhile (!waitingRequests_.empty()) {\n> -\t\tRequest *request = waitingRequests_.front();\n> -\t\tif (!request->_d()->prepared_)\n> -\t\t\tbreak;\n> +\tfor (const std::weak_ptr &ptr : cameras_) {\n\nWhy a weak_ptr ? Do we expect cameras_ to change while we're in this\nthread ? Can't this be just\n\n\tfor (auto &camera : cameras_) {\n\n\n> +\t\tstd::shared_ptr camera = ptr.lock();\n> +\t\tif (!camera)\n> +\t\t\tcontinue;\n\nI don't think cameras_ can be modified by another thread ?\n\n>\n> -\t\tdoQueueRequest(request);\n> -\t\twaitingRequests_.pop();\n> +\t\tCamera::Private *data = camera->_d();\n> +\t\twhile (!data->waitingRequests_.empty()) {\n> +\t\t\tRequest *request = data->waitingRequests_.front();\n> +\t\t\tif (!request->_d()->prepared_)\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tdoQueueRequest(request);\n> +\t\t\tdata->waitingRequests_.pop();\n> +\t\t}\n> \t}\n> }\n>\n> @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request)\n> \t\tdata->queuedRequests_.pop_front();\n> \t\tcamera->requestComplete(req);\n> \t}\n> +\n> +\tdoQueueRequests();\n> }\n>\n> /**\n> --\n> 2.43.0\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 033A6C31E9\n\tfor ;\n\tFri, 30 May 2025 09:00:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 39EA068D9E;\n\tFri, 30 May 2025 11:00:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1038614D5\n\tfor ;\n\tFri, 30 May 2025 10:59:58 +0200 (CEST)","from ideasonboard.com (mob-5-90-143-247.net.vodafone.it\n\t[5.90.143.247])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02D7089A;\n\tFri, 30 May 2025 10:59:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ib7yrSTE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1748595571;\n\tbh=S1owBYHEi+tC/lSfs1+XYrMc5bS9vZ0uJZ0a+lSRnEE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ib7yrSTEMl8cOxJEYW2liwefzAHruf5Tem7g8D0tra58O7tDO5JLbrc9uodej+F6k\n\tsZR5PdtCZF9HNxzHquvh5WPxIa4kPChadOgnLLpRu56lpbdNaDmr+YmtXP/kfhEpeH\n\tPW9ffTAWi6TiJ+9V5zCQZPrLbdSZvfMY+rIIaHVo=","Date":"Fri, 30 May 2025 10:59:55 +0200","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","Message-ID":"","References":"<20250526214224.13631-1-stefan.klug@ideasonboard.com>\n\t<20250526214224.13631-2-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250526214224.13631-2-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34623,"web_url":"https://patchwork.libcamera.org/comment/34623/","msgid":"<175077573500.1168640.1464316157619058242@localhost>","date":"2025-06-24T14:35:35","subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the review and sorry for the late feedback.\n\nQuoting Jacopo Mondi (2025-05-30 10:59:55)\n> Hi Stefan\n> \n> On Mon, May 26, 2025 at 11:42:15PM +0200, Stefan Klug wrote:\n> > The waiting requests of one camera should not be able to influence\n> > queuing to another camera. Therefore change the single waitingRequests_\n> > queue into one queue per camera.\n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > include/libcamera/internal/camera.h | 2 ++\n> > include/libcamera/internal/pipeline_handler.h | 3 --\n> > src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------\n> > 3 files changed, 25 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > index 18f5c32a18e4..8a2e9ed5894d 100644\n> > --- a/include/libcamera/internal/camera.h\n> > +++ b/include/libcamera/internal/camera.h\n> > @@ -10,6 +10,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -36,6 +37,7 @@ public:\n> > const PipelineHandler *pipe() const { return pipe_.get(); }\n> >\n> > std::list queuedRequests_;\n> > + std::queue waitingRequests_;\n> \n> So we keep the storage in Camera::Private but only access it from the\n> PipelineHandler base class ?\n> \n> I understand operating on the waitingRequst_ queue in the Camera class\n> might not be desirable, as it's way easier to do that in the\n> PipelineHandler as it runs in the library thread so we don't have to\n> worry about concurrency.\n> \n> However I'm not sure this is a good pattern, maybe we can create a map\n> in the PipelineHandler base class that associates a Camera to a queue\n> ?\n\nI was unsure about this, too. But having something like a\nstd::map> mapOfWaitingRequests_ also felt\na bit cumbersome, when there is already a internal per camera storage.\n\nI don't think the current solution is a big breach of architecture, but\nI can move it into the Pipeline handler if you want. Up to you.\n\n> \n> > ControlInfoMap controlInfo_;\n> > ControlList properties_;\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 972a2fa65310..dedc29e815fb 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -8,7 +8,6 @@\n> > #pragma once\n> >\n> > #include \n> > -#include \n> > #include \n> > #include \n> > #include \n> > @@ -94,8 +93,6 @@ private:\n> > std::vector> mediaDevices_;\n> > std::vector> cameras_;\n> >\n> > - std::queue waitingRequests_;\n> > -\n> > const char *name_;\n> > unsigned int useCount_;\n> >\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index d84dff3c9f19..14d8602e67d8 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera)\n> > /* Stop the pipeline handler and let the queued requests complete. */\n> > stopDevice(camera);\n> >\n> > + Camera::Private *data = camera->_d();\n> > +\n> > /* Cancel and signal as complete all waiting requests. */\n> > - while (!waitingRequests_.empty()) {\n> > - Request *request = waitingRequests_.front();\n> > - waitingRequests_.pop();\n> > + while (!data->waitingRequests_.empty()) {\n> > + Request *request = data->waitingRequests_.front();\n> > + data->waitingRequests_.pop();\n> > cancelRequest(request);\n> > }\n> >\n> > /* Make sure no requests are pending. */\n> > - Camera::Private *data = camera->_d();\n> > ASSERT(data->queuedRequests_.empty());\n> >\n> > data->requestSequence_ = 0;\n> > @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request)\n> > {\n> > LIBCAMERA_TRACEPOINT(request_queue, request);\n> >\n> > - waitingRequests_.push(request);\n> > + Camera *camera = request->_d()->camera();\n> > + Camera::Private *data = camera->_d();\n> > + data->waitingRequests_.push(request);\n> >\n> > request->_d()->prepare(300ms);\n> > }\n> > @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request)\n> > */\n> > void PipelineHandler::doQueueRequests()\n> \n> I've seen suggestions of passing a Camera * to this function, it might\n> remove one outer loop indeed. The function has two callers\n> \n> void PipelineHandler::completeRequest(Request *request)\n> \n> which indeed can get to Camera *\n> \n> and void Request::Private::emitPrepareCompleted()\n> \n> which can get to a Camera * as well\n> \n> > {\n> > - while (!waitingRequests_.empty()) {\n> > - Request *request = waitingRequests_.front();\n> > - if (!request->_d()->prepared_)\n> > - break;\n> > + for (const std::weak_ptr &ptr : cameras_) {\n> \n> Why a weak_ptr ? Do we expect cameras_ to change while we're in this\n> thread ? Can't this be just\n\nIf I remember correctly, I tried with auto& but then realized cameras_\nis a vector of std::weak_ptr so I removed the auto to make that\nfact clear. And afaik there is no easier way to iterate over such a\nvector than to lock every entry. Or do I miss some c++ magic here?\n\nBut anyways, as suggested by the former reviewer, the loop will vanish\nin the next version.\n\n> \n> for (auto &camera : cameras_) {\n> \n> \n> > + std::shared_ptr camera = ptr.lock();\n> > + if (!camera)\n> > + continue;\n> \n> I don't think cameras_ can be modified by another thread ?\n\nI don't think so either. But I have to properly handle the weak_ptr. Or\ndo I miss something there?\n\nBest regards,\nStefan\n\n> \n> >\n> > - doQueueRequest(request);\n> > - waitingRequests_.pop();\n> > + Camera::Private *data = camera->_d();\n> > + while (!data->waitingRequests_.empty()) {\n> > + Request *request = data->waitingRequests_.front();\n> > + if (!request->_d()->prepared_)\n> > + break;\n> > +\n> > + doQueueRequest(request);\n> > + data->waitingRequests_.pop();\n> > + }\n> > }\n> > }\n> >\n> > @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request)\n> > data->queuedRequests_.pop_front();\n> > camera->requestComplete(req);\n> > }\n> > +\n> > + doQueueRequests();\n> > }\n> >\n> > /**\n> > --\n> > 2.43.0\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 012F7BDE6B\n\tfor ;\n\tTue, 24 Jun 2025 14:35:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2171B68DF0;\n\tTue, 24 Jun 2025 16:35:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C7ADD68DE2\n\tfor ;\n\tTue, 24 Jun 2025 16:35:37 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:f370:4f13:6371:e8ac])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 1F022EFF;\n\tTue, 24 Jun 2025 16:35:20 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eb256IVC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1750775720;\n\tbh=4FCNSmS7prgkLexNLCCUt1Rv6qYfErTkvTXmQpDrfIY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=eb256IVCx1gyp9LdOu1Cyp5zqpVWfDSyzs91fHmSm++JmCenRV2FL7LXjZ1xWgW5T\n\tGosGRMOFnLEVg5fjb/O7sZxeSgPXSmCTO/c1z+clKMQ4jCM5nZVwssm8xsEBqkBT1k\n\t7S3Zknj80CR/G1C0iOLZorCqyL9pDx9g7E9zfjdM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250526214224.13631-1-stefan.klug@ideasonboard.com>\n\t<20250526214224.13631-2-stefan.klug@ideasonboard.com>\n\t","Subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","To":"Jacopo Mondi ","Date":"Tue, 24 Jun 2025 16:35:35 +0200","Message-ID":"<175077573500.1168640.1464316157619058242@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34624,"web_url":"https://patchwork.libcamera.org/comment/34624/","msgid":"<175077638993.3871677.17942807715449494310@ping.linuxembedded.co.uk>","date":"2025-06-24T14:46:29","subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-06-24 15:35:35)\n> Hi Jacopo,\n> \n> Thank you for the review and sorry for the late feedback.\n> \n> Quoting Jacopo Mondi (2025-05-30 10:59:55)\n> > Hi Stefan\n> > \n> > On Mon, May 26, 2025 at 11:42:15PM +0200, Stefan Klug wrote:\n> > > The waiting requests of one camera should not be able to influence\n> > > queuing to another camera. Therefore change the single waitingRequests_\n> > > queue into one queue per camera.\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > include/libcamera/internal/camera.h | 2 ++\n> > > include/libcamera/internal/pipeline_handler.h | 3 --\n> > > src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------\n> > > 3 files changed, 25 insertions(+), 14 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > > index 18f5c32a18e4..8a2e9ed5894d 100644\n> > > --- a/include/libcamera/internal/camera.h\n> > > +++ b/include/libcamera/internal/camera.h\n> > > @@ -10,6 +10,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -36,6 +37,7 @@ public:\n> > > const PipelineHandler *pipe() const { return pipe_.get(); }\n> > >\n> > > std::list queuedRequests_;\n> > > + std::queue waitingRequests_;\n> > \n> > So we keep the storage in Camera::Private but only access it from the\n> > PipelineHandler base class ?\n> > \n> > I understand operating on the waitingRequst_ queue in the Camera class\n> > might not be desirable, as it's way easier to do that in the\n> > PipelineHandler as it runs in the library thread so we don't have to\n> > worry about concurrency.\n> > \n> > However I'm not sure this is a good pattern, maybe we can create a map\n> > in the PipelineHandler base class that associates a Camera to a queue\n> > ?\n> \n> I was unsure about this, too. But having something like a\n> std::map> mapOfWaitingRequests_ also felt\n> a bit cumbersome, when there is already a internal per camera storage.\n> \n> I don't think the current solution is a big breach of architecture, but\n> I can move it into the Pipeline handler if you want. Up to you.\n> \n\nStiring in here - personally - I think this wait queue is a property of\nthe camera - and it's reasonable to keep it directly associated with the\ncamera.\n\nI wouldn't want to have to keep some arbitrary map of extra private\ncamera information in (the base) pipeline handler when we have a private\ncamera data which maps to this use case already.\n\n--\nKieran\n\n\n\n\n> > \n> > > ControlInfoMap controlInfo_;\n> > > ControlList properties_;\n> > >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index 972a2fa65310..dedc29e815fb 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -8,7 +8,6 @@\n> > > #pragma once\n> > >\n> > > #include \n> > > -#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -94,8 +93,6 @@ private:\n> > > std::vector> mediaDevices_;\n> > > std::vector> cameras_;\n> > >\n> > > - std::queue waitingRequests_;\n> > > -\n> > > const char *name_;\n> > > unsigned int useCount_;\n> > >\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index d84dff3c9f19..14d8602e67d8 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera)\n> > > /* Stop the pipeline handler and let the queued requests complete. */\n> > > stopDevice(camera);\n> > >\n> > > + Camera::Private *data = camera->_d();\n> > > +\n> > > /* Cancel and signal as complete all waiting requests. */\n> > > - while (!waitingRequests_.empty()) {\n> > > - Request *request = waitingRequests_.front();\n> > > - waitingRequests_.pop();\n> > > + while (!data->waitingRequests_.empty()) {\n> > > + Request *request = data->waitingRequests_.front();\n> > > + data->waitingRequests_.pop();\n> > > cancelRequest(request);\n> > > }\n> > >\n> > > /* Make sure no requests are pending. */\n> > > - Camera::Private *data = camera->_d();\n> > > ASSERT(data->queuedRequests_.empty());\n> > >\n> > > data->requestSequence_ = 0;\n> > > @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request)\n> > > {\n> > > LIBCAMERA_TRACEPOINT(request_queue, request);\n> > >\n> > > - waitingRequests_.push(request);\n> > > + Camera *camera = request->_d()->camera();\n> > > + Camera::Private *data = camera->_d();\n> > > + data->waitingRequests_.push(request);\n> > >\n> > > request->_d()->prepare(300ms);\n> > > }\n> > > @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request)\n> > > */\n> > > void PipelineHandler::doQueueRequests()\n> > \n> > I've seen suggestions of passing a Camera * to this function, it might\n> > remove one outer loop indeed. The function has two callers\n> > \n> > void PipelineHandler::completeRequest(Request *request)\n> > \n> > which indeed can get to Camera *\n> > \n> > and void Request::Private::emitPrepareCompleted()\n> > \n> > which can get to a Camera * as well\n> > \n> > > {\n> > > - while (!waitingRequests_.empty()) {\n> > > - Request *request = waitingRequests_.front();\n> > > - if (!request->_d()->prepared_)\n> > > - break;\n> > > + for (const std::weak_ptr &ptr : cameras_) {\n> > \n> > Why a weak_ptr ? Do we expect cameras_ to change while we're in this\n> > thread ? Can't this be just\n> \n> If I remember correctly, I tried with auto& but then realized cameras_\n> is a vector of std::weak_ptr so I removed the auto to make that\n> fact clear. And afaik there is no easier way to iterate over such a\n> vector than to lock every entry. Or do I miss some c++ magic here?\n> \n> But anyways, as suggested by the former reviewer, the loop will vanish\n> in the next version.\n> \n> > \n> > for (auto &camera : cameras_) {\n> > \n> > \n> > > + std::shared_ptr camera = ptr.lock();\n> > > + if (!camera)\n> > > + continue;\n> > \n> > I don't think cameras_ can be modified by another thread ?\n> \n> I don't think so either. But I have to properly handle the weak_ptr. Or\n> do I miss something there?\n> \n> Best regards,\n> Stefan\n> \n> > \n> > >\n> > > - doQueueRequest(request);\n> > > - waitingRequests_.pop();\n> > > + Camera::Private *data = camera->_d();\n> > > + while (!data->waitingRequests_.empty()) {\n> > > + Request *request = data->waitingRequests_.front();\n> > > + if (!request->_d()->prepared_)\n> > > + break;\n> > > +\n> > > + doQueueRequest(request);\n> > > + data->waitingRequests_.pop();\n> > > + }\n> > > }\n> > > }\n> > >\n> > > @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request)\n> > > data->queuedRequests_.pop_front();\n> > > camera->requestComplete(req);\n> > > }\n> > > +\n> > > + doQueueRequests();\n> > > }\n> > >\n> > > /**\n> > > --\n> > > 2.43.0\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 98004C3237\n\tfor ;\n\tTue, 24 Jun 2025 14:46:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CD3768DF0;\n\tTue, 24 Jun 2025 16:46:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9514668DE2\n\tfor ;\n\tTue, 24 Jun 2025 16:46:32 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0B8A3EFF;\n\tTue, 24 Jun 2025 16:46:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VtGPL9XK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1750776375;\n\tbh=1NRNS8pEidH+1/1lZOxxVcFCxVV/4CfLydio3OQCqyg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VtGPL9XKld0QUvCLkH5VtFYVlqvC72vSE5U6r0H0VbpY+KJ/yV027S+buH7ofNNaF\n\t8IuzAXTTnuQvaKujzF8nbb7CKxavEV03S0TwgWDbvKmeXm9b6X2JbWNc3lvEDUjIEw\n\tMrMFrEbDIuC1ez8LMNS1xusNFIDgnbUY91+q913Y=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175077573500.1168640.1464316157619058242@localhost>","References":"<20250526214224.13631-1-stefan.klug@ideasonboard.com>\n\t<20250526214224.13631-2-stefan.klug@ideasonboard.com>\n\t\n\t<175077573500.1168640.1464316157619058242@localhost>","Subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Jacopo Mondi ,\n\tStefan Klug ","Date":"Tue, 24 Jun 2025 15:46:29 +0100","Message-ID":"<175077638993.3871677.17942807715449494310@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34625,"web_url":"https://patchwork.libcamera.org/comment/34625/","msgid":"","date":"2025-06-24T15:49:50","subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran, Stefan\n\nOn Tue, Jun 24, 2025 at 03:46:29PM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2025-06-24 15:35:35)\n> > Hi Jacopo,\n> >\n> > Thank you for the review and sorry for the late feedback.\n> >\n> > Quoting Jacopo Mondi (2025-05-30 10:59:55)\n> > > Hi Stefan\n> > >\n> > > On Mon, May 26, 2025 at 11:42:15PM +0200, Stefan Klug wrote:\n> > > > The waiting requests of one camera should not be able to influence\n> > > > queuing to another camera. Therefore change the single waitingRequests_\n> > > > queue into one queue per camera.\n> > > >\n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > include/libcamera/internal/camera.h | 2 ++\n> > > > include/libcamera/internal/pipeline_handler.h | 3 --\n> > > > src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------\n> > > > 3 files changed, 25 insertions(+), 14 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > > > index 18f5c32a18e4..8a2e9ed5894d 100644\n> > > > --- a/include/libcamera/internal/camera.h\n> > > > +++ b/include/libcamera/internal/camera.h\n> > > > @@ -10,6 +10,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -36,6 +37,7 @@ public:\n> > > > const PipelineHandler *pipe() const { return pipe_.get(); }\n> > > >\n> > > > std::list queuedRequests_;\n> > > > + std::queue waitingRequests_;\n> > >\n> > > So we keep the storage in Camera::Private but only access it from the\n> > > PipelineHandler base class ?\n> > >\n> > > I understand operating on the waitingRequst_ queue in the Camera class\n> > > might not be desirable, as it's way easier to do that in the\n> > > PipelineHandler as it runs in the library thread so we don't have to\n> > > worry about concurrency.\n> > >\n> > > However I'm not sure this is a good pattern, maybe we can create a map\n> > > in the PipelineHandler base class that associates a Camera to a queue\n> > > ?\n> >\n> > I was unsure about this, too. But having something like a\n> > std::map> mapOfWaitingRequests_ also felt\n> > a bit cumbersome, when there is already a internal per camera storage.\n> >\n> > I don't think the current solution is a big breach of architecture, but\n> > I can move it into the Pipeline handler if you want. Up to you.\n> >\n>\n> Stiring in here - personally - I think this wait queue is a property of\n> the camera - and it's reasonable to keep it directly associated with the\n> camera.\n\nI agree with the association, but the queue of requests is only\naccessed from the pipeline handler base class\n\n>\n> I wouldn't want to have to keep some arbitrary map of extra private\n> camera information in (the base) pipeline handler when we have a private\n> camera data which maps to this use case already.\n\nI don't agree this is \"extra private camera information\"\n\nRequests are queued to the pipeline handler base class (which already\nhas a queue of waiting requests btw). The Camera class mostly bridges the\nqueueRequest call between the application and the library thread.\n\nBut that's not a big deal, whatever you two like the most.\n\n>\n> > >\n> > > > ControlInfoMap controlInfo_;\n> > > > ControlList properties_;\n> > > >\n> > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > index 972a2fa65310..dedc29e815fb 100644\n> > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > @@ -8,7 +8,6 @@\n> > > > #pragma once\n> > > >\n> > > > #include \n> > > > -#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -94,8 +93,6 @@ private:\n> > > > std::vector> mediaDevices_;\n> > > > std::vector> cameras_;\n> > > >\n> > > > - std::queue waitingRequests_;\n> > > > -\n> > > > const char *name_;\n> > > > unsigned int useCount_;\n> > > >\n> > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > index d84dff3c9f19..14d8602e67d8 100644\n> > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera)\n> > > > /* Stop the pipeline handler and let the queued requests complete. */\n> > > > stopDevice(camera);\n> > > >\n> > > > + Camera::Private *data = camera->_d();\n> > > > +\n> > > > /* Cancel and signal as complete all waiting requests. */\n> > > > - while (!waitingRequests_.empty()) {\n> > > > - Request *request = waitingRequests_.front();\n> > > > - waitingRequests_.pop();\n> > > > + while (!data->waitingRequests_.empty()) {\n> > > > + Request *request = data->waitingRequests_.front();\n> > > > + data->waitingRequests_.pop();\n> > > > cancelRequest(request);\n> > > > }\n> > > >\n> > > > /* Make sure no requests are pending. */\n> > > > - Camera::Private *data = camera->_d();\n> > > > ASSERT(data->queuedRequests_.empty());\n> > > >\n> > > > data->requestSequence_ = 0;\n> > > > @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request)\n> > > > {\n> > > > LIBCAMERA_TRACEPOINT(request_queue, request);\n> > > >\n> > > > - waitingRequests_.push(request);\n> > > > + Camera *camera = request->_d()->camera();\n> > > > + Camera::Private *data = camera->_d();\n> > > > + data->waitingRequests_.push(request);\n> > > >\n> > > > request->_d()->prepare(300ms);\n> > > > }\n> > > > @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request)\n> > > > */\n> > > > void PipelineHandler::doQueueRequests()\n> > >\n> > > I've seen suggestions of passing a Camera * to this function, it might\n> > > remove one outer loop indeed. The function has two callers\n> > >\n> > > void PipelineHandler::completeRequest(Request *request)\n> > >\n> > > which indeed can get to Camera *\n> > >\n> > > and void Request::Private::emitPrepareCompleted()\n> > >\n> > > which can get to a Camera * as well\n> > >\n> > > > {\n> > > > - while (!waitingRequests_.empty()) {\n> > > > - Request *request = waitingRequests_.front();\n> > > > - if (!request->_d()->prepared_)\n> > > > - break;\n> > > > + for (const std::weak_ptr &ptr : cameras_) {\n> > >\n> > > Why a weak_ptr ? Do we expect cameras_ to change while we're in this\n> > > thread ? Can't this be just\n> >\n> > If I remember correctly, I tried with auto& but then realized cameras_\n> > is a vector of std::weak_ptr so I removed the auto to make that\n> > fact clear. And afaik there is no easier way to iterate over such a\n> > vector than to lock every entry. Or do I miss some c++ magic here?\n> >\n> > But anyways, as suggested by the former reviewer, the loop will vanish\n> > in the next version.\n> >\n\nsure\n\n> > >\n> > > for (auto &camera : cameras_) {\n> > >\n> > >\n> > > > + std::shared_ptr camera = ptr.lock();\n> > > > + if (!camera)\n> > > > + continue;\n> > >\n> > > I don't think cameras_ can be modified by another thread ?\n> >\n> > I don't think so either. But I have to properly handle the weak_ptr. Or\n> > do I miss something there?\n\nYeah my point was that the check might not be necessary (however\ndoesn't hurt ?)\n\n> >\n> > Best regards,\n> > Stefan\n> >\n> > >\n> > > >\n> > > > - doQueueRequest(request);\n> > > > - waitingRequests_.pop();\n> > > > + Camera::Private *data = camera->_d();\n> > > > + while (!data->waitingRequests_.empty()) {\n> > > > + Request *request = data->waitingRequests_.front();\n> > > > + if (!request->_d()->prepared_)\n> > > > + break;\n> > > > +\n> > > > + doQueueRequest(request);\n> > > > + data->waitingRequests_.pop();\n> > > > + }\n> > > > }\n> > > > }\n> > > >\n> > > > @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > data->queuedRequests_.pop_front();\n> > > > camera->requestComplete(req);\n> > > > }\n> > > > +\n> > > > + doQueueRequests();\n> > > > }\n> > > >\n> > > > /**\n> > > > --\n> > > > 2.43.0\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 C3708BDE6B\n\tfor ;\n\tTue, 24 Jun 2025 15:49:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7E1A68DE9;\n\tTue, 24 Jun 2025 17:49:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BAD4468DE2\n\tfor ;\n\tTue, 24 Jun 2025 17:49:55 +0200 (CEST)","from ideasonboard.com (mob-5-90-136-88.net.vodafone.it\n\t[5.90.136.88])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 53B796A6;\n\tTue, 24 Jun 2025 17:49:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"m5md40sY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1750780178;\n\tbh=KvPeCDFBBhXxr566x4c3POVUycMja9cE9rgwjvEIWLI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m5md40sYt2EksDEREFynRaOs3PNiQ2LC3oUrHs37xk6kQNm2uRCI5RIHvoVXWg8kZ\n\tk+DFVCjGiiUcFBNUat/7mZfMg/uqsDwTzTporSAofVka5pOSA8O84XzvjHOhkBVHYM\n\tSoZ26FZbgQb/9eMj+xa+mUQM8GhuaYo11cc8L4e0=","Date":"Tue, 24 Jun 2025 17:49:50 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"Jacopo Mondi , \n\tStefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","Message-ID":"","References":"<20250526214224.13631-1-stefan.klug@ideasonboard.com>\n\t<20250526214224.13631-2-stefan.klug@ideasonboard.com>\n\t\n\t<175077573500.1168640.1464316157619058242@localhost>\n\t<175077638993.3871677.17942807715449494310@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<175077638993.3871677.17942807715449494310@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34714,"web_url":"https://patchwork.libcamera.org/comment/34714/","msgid":"<20250627195113.GC28633@pendragon.ideasonboard.com>","date":"2025-06-27T19:51:13","subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Jun 24, 2025 at 05:49:50PM +0200, Jacopo Mondi wrote:\n> On Tue, Jun 24, 2025 at 03:46:29PM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2025-06-24 15:35:35)\n> > > Quoting Jacopo Mondi (2025-05-30 10:59:55)\n> > > > On Mon, May 26, 2025 at 11:42:15PM +0200, Stefan Klug wrote:\n> > > > > The waiting requests of one camera should not be able to influence\n> > > > > queuing to another camera. Therefore change the single waitingRequests_\n> > > > > queue into one queue per camera.\n> > > > >\n> > > > > Signed-off-by: Stefan Klug \n> > > > > ---\n> > > > > include/libcamera/internal/camera.h | 2 ++\n> > > > > include/libcamera/internal/pipeline_handler.h | 3 --\n> > > > > src/libcamera/pipeline_handler.cpp | 34 +++++++++++++------\n> > > > > 3 files changed, 25 insertions(+), 14 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> > > > > index 18f5c32a18e4..8a2e9ed5894d 100644\n> > > > > --- a/include/libcamera/internal/camera.h\n> > > > > +++ b/include/libcamera/internal/camera.h\n> > > > > @@ -10,6 +10,7 @@\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -36,6 +37,7 @@ public:\n> > > > > const PipelineHandler *pipe() const { return pipe_.get(); }\n> > > > >\n> > > > > std::list queuedRequests_;\n> > > > > + std::queue waitingRequests_;\n> > > >\n> > > > So we keep the storage in Camera::Private but only access it from the\n> > > > PipelineHandler base class ?\n> > > >\n> > > > I understand operating on the waitingRequst_ queue in the Camera class\n> > > > might not be desirable, as it's way easier to do that in the\n> > > > PipelineHandler as it runs in the library thread so we don't have to\n> > > > worry about concurrency.\n> > > >\n> > > > However I'm not sure this is a good pattern, maybe we can create a map\n> > > > in the PipelineHandler base class that associates a Camera to a queue\n> > > > ?\n> > >\n> > > I was unsure about this, too. But having something like a\n> > > std::map> mapOfWaitingRequests_ also felt\n> > > a bit cumbersome, when there is already a internal per camera storage.\n> > >\n> > > I don't think the current solution is a big breach of architecture, but\n> > > I can move it into the Pipeline handler if you want. Up to you.\n> >\n> > Stiring in here - personally - I think this wait queue is a property of\n> > the camera - and it's reasonable to keep it directly associated with the\n> > camera.\n> \n> I agree with the association, but the queue of requests is only\n> accessed from the pipeline handler base class\n\nTrue, but it's internal to the core classes of libcamera, I don't think\nit's a big deal. I also think it's best to keep the queue in\nCamera::Private. Camera::Private and PipelineHandler are designed to\nwork together, they're not independently developed components.\n\n> > I wouldn't want to have to keep some arbitrary map of extra private\n> > camera information in (the base) pipeline handler when we have a private\n> > camera data which maps to this use case already.\n> \n> I don't agree this is \"extra private camera information\"\n> \n> Requests are queued to the pipeline handler base class (which already\n> has a queue of waiting requests btw). The Camera class mostly bridges the\n> queueRequest call between the application and the library thread.\n> \n> But that's not a big deal, whatever you two like the most.\n> \n> > > > > ControlInfoMap controlInfo_;\n> > > > > ControlList properties_;\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > > index 972a2fa65310..dedc29e815fb 100644\n> > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > @@ -8,7 +8,6 @@\n> > > > > #pragma once\n> > > > >\n> > > > > #include \n> > > > > -#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -94,8 +93,6 @@ private:\n> > > > > std::vector> mediaDevices_;\n> > > > > std::vector> cameras_;\n> > > > >\n> > > > > - std::queue waitingRequests_;\n> > > > > -\n> > > > > const char *name_;\n> > > > > unsigned int useCount_;\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > index d84dff3c9f19..14d8602e67d8 100644\n> > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > @@ -363,15 +363,16 @@ void PipelineHandler::stop(Camera *camera)\n> > > > > /* Stop the pipeline handler and let the queued requests complete. */\n> > > > > stopDevice(camera);\n> > > > >\n> > > > > + Camera::Private *data = camera->_d();\n> > > > > +\n> > > > > /* Cancel and signal as complete all waiting requests. */\n> > > > > - while (!waitingRequests_.empty()) {\n> > > > > - Request *request = waitingRequests_.front();\n> > > > > - waitingRequests_.pop();\n> > > > > + while (!data->waitingRequests_.empty()) {\n> > > > > + Request *request = data->waitingRequests_.front();\n> > > > > + data->waitingRequests_.pop();\n> > > > > cancelRequest(request);\n> > > > > }\n> > > > >\n> > > > > /* Make sure no requests are pending. */\n> > > > > - Camera::Private *data = camera->_d();\n> > > > > ASSERT(data->queuedRequests_.empty());\n> > > > >\n> > > > > data->requestSequence_ = 0;\n> > > > > @@ -444,7 +445,9 @@ void PipelineHandler::queueRequest(Request *request)\n> > > > > {\n> > > > > LIBCAMERA_TRACEPOINT(request_queue, request);\n> > > > >\n> > > > > - waitingRequests_.push(request);\n> > > > > + Camera *camera = request->_d()->camera();\n> > > > > + Camera::Private *data = camera->_d();\n> > > > > + data->waitingRequests_.push(request);\n> > > > >\n> > > > > request->_d()->prepare(300ms);\n> > > > > }\n> > > > > @@ -480,13 +483,20 @@ void PipelineHandler::doQueueRequest(Request *request)\n> > > > > */\n> > > > > void PipelineHandler::doQueueRequests()\n> > > >\n> > > > I've seen suggestions of passing a Camera * to this function, it might\n> > > > remove one outer loop indeed. The function has two callers\n> > > >\n> > > > void PipelineHandler::completeRequest(Request *request)\n> > > >\n> > > > which indeed can get to Camera *\n> > > >\n> > > > and void Request::Private::emitPrepareCompleted()\n> > > >\n> > > > which can get to a Camera * as well\n> > > >\n> > > > > {\n> > > > > - while (!waitingRequests_.empty()) {\n> > > > > - Request *request = waitingRequests_.front();\n> > > > > - if (!request->_d()->prepared_)\n> > > > > - break;\n> > > > > + for (const std::weak_ptr &ptr : cameras_) {\n> > > >\n> > > > Why a weak_ptr ? Do we expect cameras_ to change while we're in this\n> > > > thread ? Can't this be just\n> > >\n> > > If I remember correctly, I tried with auto& but then realized cameras_\n> > > is a vector of std::weak_ptr so I removed the auto to make that\n> > > fact clear. And afaik there is no easier way to iterate over such a\n> > > vector than to lock every entry. Or do I miss some c++ magic here?\n> > >\n> > > But anyways, as suggested by the former reviewer, the loop will vanish\n> > > in the next version.\n> \n> sure\n> \n> > > >\n> > > > for (auto &camera : cameras_) {\n> > > >\n> > > >\n> > > > > + std::shared_ptr camera = ptr.lock();\n> > > > > + if (!camera)\n> > > > > + continue;\n> > > >\n> > > > I don't think cameras_ can be modified by another thread ?\n> > >\n> > > I don't think so either. But I have to properly handle the weak_ptr. Or\n> > > do I miss something there?\n> \n> Yeah my point was that the check might not be necessary (however\n> doesn't hurt ?)\n> \n> > > > > - doQueueRequest(request);\n> > > > > - waitingRequests_.pop();\n> > > > > + Camera::Private *data = camera->_d();\n> > > > > + while (!data->waitingRequests_.empty()) {\n> > > > > + Request *request = data->waitingRequests_.front();\n> > > > > + if (!request->_d()->prepared_)\n> > > > > + break;\n> > > > > +\n> > > > > + doQueueRequest(request);\n> > > > > + data->waitingRequests_.pop();\n> > > > > + }\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > @@ -562,6 +572,8 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > > data->queuedRequests_.pop_front();\n> > > > > camera->requestComplete(req);\n> > > > > }\n> > > > > +\n> > > > > + doQueueRequests();\n> > > > > }\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 DFC08C3237\n\tfor ;\n\tFri, 27 Jun 2025 19:51:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9F2F68E05;\n\tFri, 27 Jun 2025 21:51:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A26E68DFD\n\tfor ;\n\tFri, 27 Jun 2025 21:51:36 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 823637E1;\n\tFri, 27 Jun 2025 21:51:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QdmAGH9G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751053876;\n\tbh=m4KBFoYaBLnLQKhTMz1B3UQJYbOYrAifXyqjy8smPmc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QdmAGH9GuNIS+6BsVpw7bn3u5D3Ak1VLOQ73/LWjTc5yi1CMXCFaU1cNacTk0FjCB\n\t0/DptZTd1m35TcCHevJY4f08+TmWsdxBG7X9QyRduFZJLrY0cnVMSDmlgSwNLjAUMK\n\t9wQ1sZIHJCZEVomdPW3rxzEIoCOaRztC8guCWqlU=","Date":"Fri, 27 Jun 2025 22:51:13 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Kieran Bingham ,\n\tStefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 1/4] libcamera: pipeline_handler: Move\n\twaitingRequests_ into camera class","Message-ID":"<20250627195113.GC28633@pendragon.ideasonboard.com>","References":"<20250526214224.13631-1-stefan.klug@ideasonboard.com>\n\t<20250526214224.13631-2-stefan.klug@ideasonboard.com>\n\t\n\t<175077573500.1168640.1464316157619058242@localhost>\n\t<175077638993.3871677.17942807715449494310@ping.linuxembedded.co.uk>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]