[{"id":31996,"web_url":"https://patchwork.libcamera.org/comment/31996/","msgid":"<27b2465e-6674-48b6-b733-59300036328b@redhat.com>","date":"2024-11-03T13:22:52","subject":"Re: [PATCH v5 2/3] libcamera: simple: Track requests in\n\tconversionQueue_","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi,\n\nOn 21-Oct-24 3:37 PM, Milan Zamazal wrote:\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> Tested-by: Kieran Bingham \n> Reviewed-by: Kieran Bingham \n\nThanks, patch looks good to me:\n\nReviewed-by: Hans de Goede \n\nRegards,\n\nHans\n\n\n\n\n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 26 ++++++++++++------------\n> 1 file changed, 13 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3ddce71d3..f3a6b1635 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -282,7 +282,11 @@ public:\n> \tstd::unique_ptr delayedCtrls_;\n> \n> \tstd::vector> conversionBuffers_;\n> -\tstd::queue> conversionQueue_;\n> +\tstruct RequestOutputs {\n> +\t\tRequest *request;\n> +\t\tstd::map outputs;\n> +\t};\n> +\tstd::queue conversionQueue_;\n> \tbool useConversion_;\n> \n> \tstd::unique_ptr converter_;\n> @@ -813,16 +817,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> \t\tif (conversionQueue_.empty())\n> \t\t\treturn;\n> \n> -\t\tRequest *request = nullptr;\n> -\t\tfor (auto &item : conversionQueue_.front()) {\n> -\t\t\tFrameBuffer *outputBuffer = item.second;\n> -\t\t\trequest = outputBuffer->request();\n> -\t\t\tpipe->completeBuffer(request, outputBuffer);\n> -\t\t}\n> +\t\tRequest *request = conversionQueue_.front().request;\n> +\t\tfor (auto &item : conversionQueue_.front().outputs)\n> +\t\t\tpipe->completeBuffer(request, item.second);\n> +\t\tpipe->completeRequest(request);\n> \t\tconversionQueue_.pop();\n> \n> -\t\tif (request)\n> -\t\t\tpipe->completeRequest(request);\n> \t\treturn;\n> \t}\n> \n> @@ -838,7 +838,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> \n> \tif (useConversion_ && !conversionQueue_.empty()) {\n> \t\tconst std::map &outputs =\n> -\t\t\tconversionQueue_.front();\n> +\t\t\tconversionQueue_.front().outputs;\n> \t\tif (!outputs.empty()) {\n> \t\t\tFrameBuffer *outputBuffer = outputs.begin()->second;\n> \t\t\tif (outputBuffer)\n> @@ -862,7 +862,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> \t\t}\n> \n> \t\tif (converter_)\n> -\t\t\tconverter_->queueBuffers(buffer, conversionQueue_.front());\n> +\t\t\tconverter_->queueBuffers(buffer, conversionQueue_.front().outputs);\n> \t\telse\n> \t\t\t/*\n> \t\t\t * request->sequence() cannot be retrieved from `buffer' inside\n> @@ -870,7 +870,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> \t\t\t * already here.\n> \t\t\t */\n> \t\t\tswIsp_->queueBuffers(request->sequence(), buffer,\n> -\t\t\t\t\t conversionQueue_.front());\n> +\t\t\t\t\t conversionQueue_.front().outputs);\n> \n> \t\tconversionQueue_.pop();\n> \t\treturn;\n> @@ -1434,7 +1434,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> \t}\n> \n> \tif (data->useConversion_) {\n> -\t\tdata->conversionQueue_.push(std::move(buffers));\n> +\t\tdata->conversionQueue_.push({ .request = request, .outputs = std::move(buffers) });\n> \t\tif (data->swIsp_)\n> \t\t\tdata->swIsp_->queueRequest(request->sequence(), request->controls());\n> \t}","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 810FCBD87C\n\tfor ;\n\tSun, 3 Nov 2024 13:23:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7781D653C1;\n\tSun, 3 Nov 2024 14:23:00 +0100 (CET)","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 DA17F653BC\n\tfor ;\n\tSun, 3 Nov 2024 14:22:58 +0100 (CET)","from mail-ed1-f69.google.com (mail-ed1-f69.google.com\n\t[209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-625-Ogj9Xq1kN6S8hn5VB-Jcyw-1; Sun, 03 Nov 2024 08:22:56 -0500","by mail-ed1-f69.google.com with SMTP id\n\t4fb4d7f45d1cf-5cbad9b3ca2so2502077a12.3\n\tfor ;\n\tSun, 03 Nov 2024 05:22:56 -0800 (PST)","from ?IPV6:2001:1c00:c32:7800:5bfa:a036:83f0:f9ec?\n\t(2001-1c00-0c32-7800-5bfa-a036-83f0-f9ec.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c00:c32:7800:5bfa:a036:83f0:f9ec])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5ceac74cb4fsm3465797a12.4.2024.11.03.05.22.52\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tSun, 03 Nov 2024 05:22:53 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Lwo9X8E5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1730640177;\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\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=CExqbNsHhUg2eVNyTeIwmnqI3zD+BOBfoXxUZ0WSc18=;\n\tb=Lwo9X8E5kt8bfSJPlczC+qSFGH+s4vR3O9xdW1IuUWlW22uwp7TgixnpWlsKi0GEYL+ktw\n\tr2jG6UEGTcO3R9j8jBrhyjDujwQ7weDTIe47cspw7PzZ7gK47fQRkq5fhCl891SKMMvoUG\n\tvrGisVGjWO6btyQEQ5qS8mFq5EOwW3Q=","X-MC-Unique":"Ogj9Xq1kN6S8hn5VB-Jcyw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730640175; x=1731244975;\n\th=content-transfer-encoding:in-reply-to:from:content-language\n\t:references:cc:to:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=CExqbNsHhUg2eVNyTeIwmnqI3zD+BOBfoXxUZ0WSc18=;\n\tb=XRxAPMhBHWlPpowxIOyYEaVkQoyiTXUDvBtWKlHFvD691vF2uJdO0/p0LJS2MOphCR\n\tnYC95h8U7QCF+kEX8FOs0OffFG49YPRr0P9Lqp/gUa3v6Hmzc01opCGn8dnnt1b+dwYT\n\tteF9eZmHDQbITpEcTFwugtB4OKmc15kbq3q/0Pypxrx/5XRCseDQ7eFwb3BXM6+mSKAW\n\t8LPpO1xP+zmoeJceG/WLlRLkJBo+aYvqip7pZGZ/Tj+tr0LqCVgwxBjQlwV3w61f7Sy/\n\tnZroBhttNcF7jQqepJ3fs7yO3PuBwxrsn2phKydae1omHa3MKM0uXVr/ZLVZZQlsb88w\n\tQcmA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVvt9Li0RVvb49bn1dzZ3Tmp7Vpop0H3D/L7TNWHoqRyz7PqOhZVLNS/tgsPVa6U6x63jt+6hpuI1u4Xs859ys=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YxhrsUWhwAp1kC17daSSFh4n7MBcvOoY6cLYj1CB0puf9luWES7\n\tCRALmAtJrPtHjQvVpbLndgcX44s2nTXSAIEqDxhB5kiXFW2EUOqld1VF/s9QRjXZJH04g9YiQ14\n\tCLzwIy2Mjg8I47WQIMRUlgnD26MFIHLe/XBxIOOFAhtAp3sukgP3QSdSdyANfPEu33Al8YeM=","X-Received":["by 2002:a50:cc0d:0:b0:5cb:666e:9e4b with SMTP id\n\t4fb4d7f45d1cf-5cbbf8de134mr18388384a12.20.1730640175176; \n\tSun, 03 Nov 2024 05:22:55 -0800 (PST)","by 2002:a50:cc0d:0:b0:5cb:666e:9e4b with SMTP id\n\t4fb4d7f45d1cf-5cbbf8de134mr18388367a12.20.1730640174676; \n\tSun, 03 Nov 2024 05:22:54 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IEPegj6RfXsuD2dPVSCe9nhMqvBONkvs9SuRmgYxejzx7EI38vLsRkNoRHqZnmYYbm8MZDUng==","Message-ID":"<27b2465e-6674-48b6-b733-59300036328b@redhat.com>","Date":"Sun, 3 Nov 2024 14:22:52 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 2/3] libcamera: simple: Track requests in\n\tconversionQueue_","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Cc":"robert.mader@posteo.de, kieran.bingham@ideasonboard.com,\n\tlaurent.pinchart@ideasonboard.com","References":"<20241021133718.894374-1-mzamazal@redhat.com>\n\t<20241021133718.894374-3-mzamazal@redhat.com>","From":"Hans de Goede ","In-Reply-To":"<20241021133718.894374-3-mzamazal@redhat.com>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US, nl","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","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":32045,"web_url":"https://patchwork.libcamera.org/comment/32045/","msgid":"<20241106114512.GA24752@pendragon.ideasonboard.com>","date":"2024-11-06T11:45:12","subject":"Re: [PATCH v5 2/3] libcamera: simple: Track requests in\n\tconversionQueue_","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Mon, Oct 21, 2024 at 03:37:17PM +0200, Milan Zamazal wrote:\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> Tested-by: Kieran Bingham \n> Reviewed-by: Kieran Bingham \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 26 ++++++++++++------------\n> 1 file changed, 13 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 3ddce71d3..f3a6b1635 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -282,7 +282,11 @@ public:\n> \tstd::unique_ptr delayedCtrls_;\n> \n> \tstd::vector> conversionBuffers_;\n> -\tstd::queue> conversionQueue_;\n> +\tstruct RequestOutputs {\n> +\t\tRequest *request;\n> +\t\tstd::map outputs;\n\nI would have named this buffers, but I don't want to bikeshed on the\nname.\n\n> +\t};\n> +\tstd::queue conversionQueue_;\n> \tbool useConversion_;\n> \n> \tstd::unique_ptr converter_;\n> @@ -813,16 +817,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> \t\tif (conversionQueue_.empty())\n> \t\t\treturn;\n> \n> -\t\tRequest *request = nullptr;\n> -\t\tfor (auto &item : conversionQueue_.front()) {\n> -\t\t\tFrameBuffer *outputBuffer = item.second;\n> -\t\t\trequest = outputBuffer->request();\n> -\t\t\tpipe->completeBuffer(request, outputBuffer);\n> -\t\t}\n> +\t\tRequest *request = conversionQueue_.front().request;\n> +\t\tfor (auto &item : conversionQueue_.front().outputs)\n> +\t\t\tpipe->completeBuffer(request, item.second);\n> +\t\tpipe->completeRequest(request);\n> \t\tconversionQueue_.pop();\n\nWould the following be clearer ? I always find .first and .second\nconfusing.\n\n\t\tconst RequestOutputs &outputs = conversionQueue_.front();\n\t\tfor (auto &[stream, buffer] : outputs.outputs)\n\t\t\tpipe->completeBuffer(outputs.request, buffer);\n\t\tpipe->completeRequest(outputs.request);\n\t\tconversionQueue_.pop();\n\n> \n> -\t\tif (request)\n> -\t\t\tpipe->completeRequest(request);\n> \t\treturn;\n> \t}\n> \n> @@ -838,7 +838,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> \n> \tif (useConversion_ && !conversionQueue_.empty()) {\n> \t\tconst std::map &outputs =\n> -\t\t\tconversionQueue_.front();\n> +\t\t\tconversionQueue_.front().outputs;\n> \t\tif (!outputs.empty()) {\n> \t\t\tFrameBuffer *outputBuffer = outputs.begin()->second;\n> \t\t\tif (outputBuffer)\n> @@ -862,7 +862,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> \t\t}\n> \n> \t\tif (converter_)\n> -\t\t\tconverter_->queueBuffers(buffer, conversionQueue_.front());\n> +\t\t\tconverter_->queueBuffers(buffer, conversionQueue_.front().outputs);\n> \t\telse\n> \t\t\t/*\n> \t\t\t * request->sequence() cannot be retrieved from `buffer' inside\n> @@ -870,7 +870,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> \t\t\t * already here.\n> \t\t\t */\n> \t\t\tswIsp_->queueBuffers(request->sequence(), buffer,\n> -\t\t\t\t\t conversionQueue_.front());\n> +\t\t\t\t\t conversionQueue_.front().outputs);\n> \n> \t\tconversionQueue_.pop();\n> \t\treturn;\n> @@ -1434,7 +1434,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> \t}\n> \n> \tif (data->useConversion_) {\n> -\t\tdata->conversionQueue_.push(std::move(buffers));\n> +\t\tdata->conversionQueue_.push({ .request = request, .outputs = std::move(buffers) });\n\nDesignated initializers are a C++20 feature, you should use\n\n\t\tdata->conversionQueue_.push({ request, std::move(buffers) });\n\nWith this,\n\nReviewed-by: Laurent Pinchart \n\n> \t\tif (data->swIsp_)\n> \t\t\tdata->swIsp_->queueRequest(request->sequence(), request->controls());\n> \t}","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 240B1BDB13\n\tfor ;\n\tWed, 6 Nov 2024 11:45:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4329F6541D;\n\tWed, 6 Nov 2024 12:45:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 52491653C5\n\tfor ;\n\tWed, 6 Nov 2024 12:45:18 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E122B475;\n\tWed, 6 Nov 2024 12:45:09 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wjl9PjbW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730893510;\n\tbh=1cpJ1CaRYUfd8HQTAYyFNF+AXIwUmjBPrFe3RHLKDSk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wjl9PjbWWngQentLFufx/V/3VWv4FumnleIf2z/FjzOtShooB3rQfPpuwPLPL4bjl\n\ttsqR0J0zoMMtDslTXJZSuDc2xFci/EX2SKrqsg+gleBEwJcETUUgO5S24QKOyTEvZl\n\tZG1etJoyGS7Wfpu/DjdW2iqfSEKvb5+yXuycGG7U=","Date":"Wed, 6 Nov 2024 13:45:12 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, robert.mader@posteo.de,\n\tkieran.bingham@ideasonboard.com","Subject":"Re: [PATCH v5 2/3] libcamera: simple: Track requests in\n\tconversionQueue_","Message-ID":"<20241106114512.GA24752@pendragon.ideasonboard.com>","References":"<20241021133718.894374-1-mzamazal@redhat.com>\n\t<20241021133718.894374-3-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241021133718.894374-3-mzamazal@redhat.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\" "}}]