[{"id":31807,"web_url":"https://patchwork.libcamera.org/comment/31807/","msgid":"<172928707761.532453.10059829205706802495@ping.linuxembedded.co.uk>","date":"2024-10-18T21:31:17","subject":"Re: [PATCH v4 2/3] libcamera: simple: Track requests in\n\tconversionQueue_","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2024-10-18 10:27:15)\n> Simple pipeline retrieves the requests to complete from the\n> conversionQueue_. This patch stores the requests in conversionQueue_\n> explicitly. This explicit tracking is supposed to be preferred to\n> implicit retrieval and it simplifies the completion code a bit here and\n> in the followup patch that adds request cleanup on stop.\n> \n> The change as implemented assumes that all the buffers in each of the\n> conversionQueue_ elements point to the same request, the one specified.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 22 +++++++++-------------\n> 1 file changed, 9 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3ddce71d3..a1339d87c 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -282,7 +282,7 @@ public:\n> std::unique_ptr delayedCtrls_;\n> \n> std::vector> conversionBuffers_;\n> - std::queue> conversionQueue_;\n> + std::queue>> conversionQueue_;\n> bool useConversion_;\n> \n> std::unique_ptr converter_;\n> @@ -813,16 +813,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> if (conversionQueue_.empty())\n> return;\n> \n> - Request *request = nullptr;\n> - for (auto &item : conversionQueue_.front()) {\n> - FrameBuffer *outputBuffer = item.second;\n> - request = outputBuffer->request();\n> - pipe->completeBuffer(request, outputBuffer);\n> - }\n> + Request *request = conversionQueue_.front().first;\n> + for (auto &item : conversionQueue_.front().second)\n> + pipe->completeBuffer(request, item.second);\n\nI'll always have a personal dislike for std::pair<> because\nsomething.first and something.second just really doesn't tell me what\nit's supposed to be.\n\nEven more so with\n\t\"auto &item : conversionQueue_.front().second\"\n\n\n\n\n> + pipe->completeRequest(request);\n> conversionQueue_.pop();\n> \n> - if (request)\n> - pipe->completeRequest(request);\n> return;\n> }\n> \n> @@ -838,7 +834,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> \n> if (useConversion_ && !conversionQueue_.empty()) {\n> const std::map &outputs =\n> - conversionQueue_.front();\n> + conversionQueue_.front().second;\n> if (!outputs.empty()) {\n> FrameBuffer *outputBuffer = outputs.begin()->second;\n> if (outputBuffer)\n> @@ -862,7 +858,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> }\n> \n> if (converter_)\n> - converter_->queueBuffers(buffer, conversionQueue_.front());\n> + converter_->queueBuffers(buffer, conversionQueue_.front().second);\n> else\n> /*\n> * request->sequence() cannot be retrieved from `buffer' inside\n> @@ -870,7 +866,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> * already here.\n> */\n> swIsp_->queueBuffers(request->sequence(), buffer,\n> - conversionQueue_.front());\n> + conversionQueue_.front().second);\n> \n> conversionQueue_.pop();\n> return;\n> @@ -1434,7 +1430,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> }\n> \n> if (data->useConversion_) {\n> - data->conversionQueue_.push(std::move(buffers));\n> + data->conversionQueue_.push(std::make_pair(request, std::move(buffers)));\n\nAs far as I can tell all the above looks correct, even with my dislike\nof std::pair - and it's /working/\n\nTested-by: Kieran Bingham \n\n\nIf there were a reason to respin, and std::pair could be more readable\nI'd be happier - but I can still do this I think:\n\n\nReviewed-by: Kieran Bingham \n\n> if (data->swIsp_)\n> data->swIsp_->queueRequest(request->sequence(), request->controls());\n> }\n> -- \n> 2.44.1\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 BD1E0C32AF\n\tfor ;\n\tFri, 18 Oct 2024 21:31:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 641AE6538B;\n\tFri, 18 Oct 2024 23:31:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3F53D633C7\n\tfor ;\n\tFri, 18 Oct 2024 23:31:21 +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 06B92268;\n\tFri, 18 Oct 2024 23:29: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=\"CNmgKYOe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1729286977;\n\tbh=RKo+39/JnoM05ttP5z/LLQjnv/fQ4bBF4R/XH2KnEVI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=CNmgKYOeYFBIYaNxR8xluMdb6qsyIMG/Lbu4ubqAaUSWiVz8xAlgcyBeAW0BgSeSU\n\tvuT5XoJMts2mxL5ktxx6Pl8ONd3+9s0tu1Y5l4s3emeYVuTZl48VX6WAMH0tnsVT0G\n\tdNd8viNV+a18hV+iAcTtVSPrHZl6A0LM5SnSkql0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20241018092716.295624-3-mzamazal@redhat.com>","References":"<20241018092716.295624-1-mzamazal@redhat.com>\n\t<20241018092716.295624-3-mzamazal@redhat.com>","Subject":"Re: [PATCH v4 2/3] libcamera: simple: Track requests in\n\tconversionQueue_","From":"Kieran Bingham ","Cc":"Milan Zamazal , robert.mader@posteo.de,\n\tlaurent.pinchart@ideasonboard.com","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Fri, 18 Oct 2024 22:31:17 +0100","Message-ID":"<172928707761.532453.10059829205706802495@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":31855,"web_url":"https://patchwork.libcamera.org/comment/31855/","msgid":"<87msixd1yr.fsf@redhat.com>","date":"2024-10-21T13:23:24","subject":"Re: [PATCH v4 2/3] libcamera: simple: Track requests in\n\tconversionQueue_","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Kieran Bingham writes:\n\n> Quoting Milan Zamazal (2024-10-18 10:27:15)\n>> Simple pipeline retrieves the requests to complete from the\n>> conversionQueue_. This patch stores the requests in conversionQueue_\n>\n>> explicitly. This explicit tracking is supposed to be preferred to\n>> implicit retrieval and it simplifies the completion code a bit here and\n>> in the followup patch that adds request cleanup on stop.\n>> \n>> The change as implemented assumes that all the buffers in each of the\n>> conversionQueue_ elements point to the same request, the one specified.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> src/libcamera/pipeline/simple/simple.cpp | 22 +++++++++-------------\n>> 1 file changed, 9 insertions(+), 13 deletions(-)\n>> \n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 3ddce71d3..a1339d87c 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -282,7 +282,7 @@ public:\n>> std::unique_ptr delayedCtrls_;\n>> \n>> std::vector> conversionBuffers_;\n>> - std::queue> conversionQueue_;\n>> + std::queue>> conversionQueue_;\n>> bool useConversion_;\n>> \n>> std::unique_ptr converter_;\n>> @@ -813,16 +813,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n>> if (conversionQueue_.empty())\n>> return;\n>> \n>> - Request *request = nullptr;\n>> - for (auto &item : conversionQueue_.front()) {\n>> - FrameBuffer *outputBuffer = item.second;\n>> - request = outputBuffer->request();\n>> - pipe->completeBuffer(request, outputBuffer);\n>> - }\n>> + Request *request = conversionQueue_.front().first;\n>> + for (auto &item : conversionQueue_.front().second)\n>> + pipe->completeBuffer(request, item.second);\n>\n> I'll always have a personal dislike for std::pair<> because\n> something.first and something.second just really doesn't tell me what\n> it's supposed to be.\n>\n> Even more so with\n> \t\"auto &item : conversionQueue_.front().second\"\n>\n>\n>\n>\n>> + pipe->completeRequest(request);\n>> conversionQueue_.pop();\n>> \n>> - if (request)\n>> - pipe->completeRequest(request);\n>> return;\n>> }\n>> \n>> @@ -838,7 +834,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n>> \n>> if (useConversion_ && !conversionQueue_.empty()) {\n>> const std::map &outputs =\n>> - conversionQueue_.front();\n>> + conversionQueue_.front().second;\n>> if (!outputs.empty()) {\n>> FrameBuffer *outputBuffer = outputs.begin()->second;\n>> if (outputBuffer)\n>> @@ -862,7 +858,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n>> }\n>> \n>> if (converter_)\n>> - converter_->queueBuffers(buffer, conversionQueue_.front());\n>> + converter_->queueBuffers(buffer, conversionQueue_.front().second);\n>> else\n>> /*\n>> * request->sequence() cannot be retrieved from `buffer' inside\n>> @@ -870,7 +866,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n>> * already here.\n>> */\n>> swIsp_->queueBuffers(request->sequence(), buffer,\n>> - conversionQueue_.front());\n>> + conversionQueue_.front().second);\n>> \n>> conversionQueue_.pop();\n>> return;\n>> @@ -1434,7 +1430,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n>> }\n>> \n>> if (data->useConversion_) {\n>> - data->conversionQueue_.push(std::move(buffers));\n>> + data->conversionQueue_.push(std::make_pair(request, std::move(buffers)));\n>\n> As far as I can tell all the above looks correct, even with my dislike\n> of std::pair - and it's /working/\n>\n> Tested-by: Kieran Bingham \n>\n>\n> If there were a reason to respin, and std::pair could be more readable\n> I'd be happier - but I can still do this I think:\n\nLet's make you happier, this is a safe and easy change, I'll post v5\nwith it.\n\n> Reviewed-by: Kieran Bingham \n>\n>> if (data->swIsp_)\n>> data->swIsp_->queueRequest(request->sequence(), request->controls());\n>> }\n>> -- \n>> 2.44.1\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 44BBEBD808\n\tfor ;\n\tMon, 21 Oct 2024 13:23:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 350EE65392;\n\tMon, 21 Oct 2024 15:23:32 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F6C86538A\n\tfor ;\n\tMon, 21 Oct 2024 15:23:30 +0200 (CEST)","from mail-wr1-f69.google.com (mail-wr1-f69.google.com\n\t[209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-511-8hTeXHRlOq-UieEqvNC3Qg-1; Mon, 21 Oct 2024 09:23:27 -0400","by mail-wr1-f69.google.com with SMTP id\n\tffacd0b85a97d-37d49887a2cso2449571f8f.0\n\tfor ;\n\tMon, 21 Oct 2024 06:23:27 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-4316f5cc5adsm57415325e9.42.2024.10.21.06.23.24\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 21 Oct 2024 06:23:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"EJYTuzqb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1729517009;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=O2rwRtswhq0QkegnjhB6tBqQuMw5VyCXBjvaQDMiUT0=;\n\tb=EJYTuzqbwXqfvyHfoYnJdL7C470o9RSxdlD0HNfi0o+W7UIXtdl7HIUNnnhLp4jYJUbUoL\n\t6GX2OIlTryYNyopgsOqc4l+nqhoDHuYALPxbpKrgdsFKh8bkJ5AdyJdaB4v5j+MOimDc0N\n\tNC7re4fZ4YRMifzOsJ2HPbqW99SYpjo=","X-MC-Unique":"8hTeXHRlOq-UieEqvNC3Qg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1729517006; x=1730121806;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=O2rwRtswhq0QkegnjhB6tBqQuMw5VyCXBjvaQDMiUT0=;\n\tb=KaW0FML1ICbQUIWXxh3JjBnBkVhMu8Gmkx2mXV6s2OGE6NRE/L1QXktfN2SE1FC8aC\n\tZbNThARxA4k22rJjG1s1YWyalCBdtJyFFUEBKZKrS3YwN7lBs0ztnXansjtyvLazM3H6\n\t3+9yBoEh+MQD5aFWap8zGEq9DR+O3DfU8uwtifwLdwLNIj8qoWNdhfSMMtoJMz3wIedu\n\tDaLYbMXwMF/LNAlcqAvu2IorwAXUe/nTmdWXrCBb/u6VbwLTuNoSa/1MK6OL+mBHP9ud\n\tZq5cHWMowEr32VOn65lTjKPnxEdPmniHtCTGlPkimf9HnYyqIx7l64TTRGTu5AsHTbWk\n\tResg==","X-Gm-Message-State":"AOJu0Yz8/54dCLRF1+Ye382b8vTkiN6cAgQm5bHul8nqLDB0WzArSnm1\n\tGPnmsdNfMjUzcyIbvaMPspC+g+aQ3qg/Y5G06U1gGsoYX1TNrJTDfOiapp3WPHUOkKxdifD0cpG\n\tvPUc3EV9WPV/zRn8jxVHAuQUH91rB7xoroB8xIVe9oZVXwzVRxGLdjG3D/TeEtt0VpiQKTm8=","X-Received":["by 2002:a05:6000:e83:b0:374:cee6:c298 with SMTP id\n\tffacd0b85a97d-37d93e2492bmr9970566f8f.21.1729517006714; \n\tMon, 21 Oct 2024 06:23:26 -0700 (PDT)","by 2002:a05:6000:e83:b0:374:cee6:c298 with SMTP id\n\tffacd0b85a97d-37d93e2492bmr9970549f8f.21.1729517006337; \n\tMon, 21 Oct 2024 06:23:26 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEh+bfTFkxiXdFokvYFa191U69kVJphddXK0rQrRgfb94vHZZnKHAeSgEdgsfHXNWe3Y/rn9w==","From":"Milan Zamazal ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tlaurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH v4 2/3] libcamera: simple: Track requests in\n\tconversionQueue_","In-Reply-To":"<172928707761.532453.10059829205706802495@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Fri, 18 Oct 2024 22:31:17 +0100\")","References":"<20241018092716.295624-1-mzamazal@redhat.com>\n\t<20241018092716.295624-3-mzamazal@redhat.com>\n\t<172928707761.532453.10059829205706802495@ping.linuxembedded.co.uk>","Date":"Mon, 21 Oct 2024 15:23:24 +0200","Message-ID":"<87msixd1yr.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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\" "}}]