[{"id":33244,"web_url":"https://patchwork.libcamera.org/comment/33244/","msgid":"<20250131192945.GJ12673@pendragon.ideasonboard.com>","date":"2025-01-31T19:29:45","subject":"Re: [PATCH v2] libcamera: software_isp: Handle signals in the proper\n\tthread","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 Fri, Jan 31, 2025 at 08:18:38PM +0100, Milan Zamazal wrote:\n> inputBufferReady ready signal in the simple pipeline is handled in the\n> pipeline handler thread. outputBufferReady and ispStatsReady signals\n> should be handled there too.\n> \n> Rather than relying on the user of the SoftwareIsp instance, let\n> SoftwareIsp inherits Object. SoftwareIsp serves as a signal proxy, the\n> signals above are emitted from signal handlers. This means that if\n> SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp\n> thread. Which is the camera manager thread because the SoftwareIsp\n> instance is created there.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> .../internal/software_isp/software_isp.h | 3 ++-\n> src/libcamera/pipeline/simple/simple.cpp | 19 ++++++-------------\n> 2 files changed, 8 insertions(+), 14 deletions(-)\n> \n> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> index d51b03fd..440a296d 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -18,6 +18,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -43,7 +44,7 @@ struct StreamConfiguration;\n> \n> LOG_DECLARE_CATEGORY(SoftwareIsp)\n> \n> -class SoftwareIsp\n> +class SoftwareIsp : public Object\n> {\n> public:\n> \tSoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 8ac24e6e..fade8fda 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -538,20 +538,13 @@ int SimpleCameraData::init()\n> \t\t\tswIsp_.reset();\n> \t\t} else {\n> \t\t\t/*\n> -\t\t\t * The inputBufferReady signal is emitted from the soft ISP thread,\n> -\t\t\t * and needs to be handled in the pipeline handler thread. Signals\n> -\t\t\t * implement queued delivery, but this works transparently only if\n> -\t\t\t * the receiver is bound to the target thread. As the\n> -\t\t\t * SimpleCameraData class doesn't inherit from the Object class, it\n> -\t\t\t * is not bound to any thread, and the signal would be delivered\n> -\t\t\t * synchronously. Instead, connect the signal to a lambda function\n> -\t\t\t * bound explicitly to the pipe, which is bound to the pipeline\n> -\t\t\t * handler thread. The function then simply forwards the call to\n> -\t\t\t * conversionInputDone().\n> +\t\t\t * The connected signals should be handled by the camera manager\n> +\t\t\t * thread. This method is called in the camera manager thread and\n> +\t\t\t * instantiates the SoftwareIsp instance, which inherits Object and\n> +\t\t\t * emits the signals from the instance's own signal handlers; thus\n> +\t\t\t * the slots here are invoked in the camera manager thread.\n> \t\t\t */\n\nI think you can drop this comment complete. With that,\n\nReviewed-by: Laurent Pinchart \n\n> -\t\t\tswIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) {\n> -\t\t\t\tthis->conversionInputDone(buffer);\n> -\t\t\t});\n> +\t\t\tswIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n> \t\t\tswIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n> \t\t\tswIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n> \t\t\tswIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);","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 3F7BAC3263\n\tfor ;\n\tFri, 31 Jan 2025 19:29:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 996B56856C;\n\tFri, 31 Jan 2025 20:29:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E8E160353\n\tfor ;\n\tFri, 31 Jan 2025 20:29:49 +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 0A702465;\n\tFri, 31 Jan 2025 20:28:39 +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=\"OcGG5hdr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738351720;\n\tbh=Fp0RrKJsTNLK9yDMCOEikWDgZFzi0t7FllWpJsOQFHE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OcGG5hdrdTFfVfzC5aXa6H9bJnFVQK+h3BbYW/ZEpISm98Aiyy6UGJlVaBPUPqXfR\n\tEfN4Cu2PsQwOgiQFL3V7+I6qHvBuA6mESUT7tHvjKDt6CIz/ToSX53gFhfOw+jOQlI\n\tB+tuY835t+/exs/nQ7Uh9rVaiDGK1DsApHT0z6nU=","Date":"Fri, 31 Jan 2025 21:29:45 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, Stanislaw Gruszka\n\t, =?utf-8?b?QmFybmFiw6FzIFDFkWN6?=\n\t=?utf-8?q?e?= ","Subject":"Re: [PATCH v2] libcamera: software_isp: Handle signals in the proper\n\tthread","Message-ID":"<20250131192945.GJ12673@pendragon.ideasonboard.com>","References":"<20250131191838.47661-1-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250131191838.47661-1-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\" "}},{"id":33245,"web_url":"https://patchwork.libcamera.org/comment/33245/","msgid":"<85lduqwy7c.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-01-31T20:00:23","subject":"Re: [PATCH v2] libcamera: software_isp: Handle signals in the\n\tproper thread","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nthank you for review.\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Fri, Jan 31, 2025 at 08:18:38PM +0100, Milan Zamazal wrote:\n>> inputBufferReady ready signal in the simple pipeline is handled in the\n>> pipeline handler thread. outputBufferReady and ispStatsReady signals\n>> should be handled there too.\n>> \n>> Rather than relying on the user of the SoftwareIsp instance, let\n>> SoftwareIsp inherits Object. SoftwareIsp serves as a signal proxy, the\n>> signals above are emitted from signal handlers. This means that if\n>> SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp\n>> thread. Which is the camera manager thread because the SoftwareIsp\n>> instance is created there.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> .../internal/software_isp/software_isp.h | 3 ++-\n>> src/libcamera/pipeline/simple/simple.cpp | 19 ++++++-------------\n>> 2 files changed, 8 insertions(+), 14 deletions(-)\n>> \n>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n>> index d51b03fd..440a296d 100644\n>> --- a/include/libcamera/internal/software_isp/software_isp.h\n>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>> @@ -18,6 +18,7 @@\n>> \n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>> \n>> @@ -43,7 +44,7 @@ struct StreamConfiguration;\n>> \n>> LOG_DECLARE_CATEGORY(SoftwareIsp)\n>> \n>> -class SoftwareIsp\n>> +class SoftwareIsp : public Object\n>> {\n>> public:\n>> \tSoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 8ac24e6e..fade8fda 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -538,20 +538,13 @@ int SimpleCameraData::init()\n>> \t\t\tswIsp_.reset();\n>> \t\t} else {\n>> \t\t\t/*\n>> -\t\t\t * The inputBufferReady signal is emitted from the soft ISP thread,\n>> -\t\t\t * and needs to be handled in the pipeline handler thread. Signals\n>> -\t\t\t * implement queued delivery, but this works transparently only if\n>> -\t\t\t * the receiver is bound to the target thread. As the\n>> -\t\t\t * SimpleCameraData class doesn't inherit from the Object class, it\n>> -\t\t\t * is not bound to any thread, and the signal would be delivered\n>> -\t\t\t * synchronously. Instead, connect the signal to a lambda function\n>> -\t\t\t * bound explicitly to the pipe, which is bound to the pipeline\n>> -\t\t\t * handler thread. The function then simply forwards the call to\n>> -\t\t\t * conversionInputDone().\n>> +\t\t\t * The connected signals should be handled by the camera manager\n>> +\t\t\t * thread. This method is called in the camera manager thread and\n>> +\t\t\t * instantiates the SoftwareIsp instance, which inherits Object and\n>> +\t\t\t * emits the signals from the instance's own signal handlers; thus\n>> +\t\t\t * the slots here are invoked in the camera manager thread.\n>> \t\t\t */\n>\n> I think you can drop this comment complete. With that,\n\nOK, posted v3 with this change.\n\n> Reviewed-by: Laurent Pinchart \n>\n>> -\t\t\tswIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) {\n>> -\t\t\t\tthis->conversionInputDone(buffer);\n>> -\t\t\t});\n>> +\t\t\tswIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n>> \t\t\tswIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n>> \t\t\tswIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n>> \t\t\tswIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);","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 52919BD808\n\tfor ;\n\tFri, 31 Jan 2025 20:00:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B20A6856E;\n\tFri, 31 Jan 2025 21:00:34 +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 AAB9668563\n\tfor ;\n\tFri, 31 Jan 2025 21:00:31 +0100 (CET)","from mail-wm1-f69.google.com (mail-wm1-f69.google.com\n\t[209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-187-oKmmbL7MOb-MJ1jHGnW16Q-1; Fri, 31 Jan 2025 15:00:26 -0500","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-43624b08181so11632135e9.0\n\tfor ;\n\tFri, 31 Jan 2025 12:00:26 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb ([77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-38c5c11b363sm5427989f8f.40.2025.01.31.12.00.24\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 31 Jan 2025 12:00:24 -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=\"IALH7Mpl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1738353630;\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=5+SctQ/E6ZcqQhOWhp6qR2f7BP0exrfXMjqCdSHg20I=;\n\tb=IALH7MplK8+WUd4jp/635HWMNZEnXQCf8QdXRyzPWhHuuz62vLvp2bePHET2AcWja8eKrN\n\tFS1JCoESPsaAapJcydzLpSRiLHFojgY6hDUSF/9ar1zBLDkfnTSkjnz4FXpRO/1b9wXv30\n\tuOyl2Yz28G8nxARfh0FPvHSIUaDnLP4=","X-MC-Unique":"oKmmbL7MOb-MJ1jHGnW16Q-1","X-Mimecast-MFC-AGG-ID":"oKmmbL7MOb-MJ1jHGnW16Q","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738353625; x=1738958425;\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=5+SctQ/E6ZcqQhOWhp6qR2f7BP0exrfXMjqCdSHg20I=;\n\tb=OI1+Eltq88DXJpp71ZkNjbyO/8KrQEMrFc3jiSGrD1xLcpGEL1FGaJOKtDjG4NmNz8\n\tEnrv26DUbcBN7hEL+jSI/eqxOh98IHmMLpJGJ6HdMN2ZNxiU1sZHUbIPNFrCrEitR/uB\n\tuEs3GUUUBndHeTR2Iu1Ami2+tL33PMNSlN1vJhZ7xYXNXwm+HtaguHF+6gF0aZkSKznr\n\tR0VQNcUhzdFryJHx3OR5xXfFj8qXctZ/GUp2Q75yBqIPp86tdAdNV6Mw4oaXksGw6kAE\n\trVdkAyZMu1rSVQXgeY3F6q55kUytwUCjpsflUqEkEC0W0O6Zb3rwRCJjPFr1jVGpbv0P\n\tAogg==","X-Gm-Message-State":"AOJu0YzQe7S+KNyXeCE7GRnL6pOuY5Vd/GjPIKIcGO3KJUygPgJ7auRP\n\t6lidT4YrmSDqsRxHswhfM/KuFs+zkBjatlQa1raNb8xE0WXkM7+BZH6lrxi5kHzqwsXiPJxOpE/\n\tKP5RR9VclSHdLDHMI/w9KFE6Ml2AfRkNgMXV+ERRFK39IwqylAJC5XDJ9mqYV62jeW79Fzvc=","X-Gm-Gg":"ASbGnctP68pSSmBKQvQzyir8Xmvw2KPRDw96TvXwk//f+BQTPZP3Vk6X5UZn5GPAs+C\n\t4lPGC/B0QNrq02TEZb4fyk3CSll3URp5Gyizk9HAE77FmlHFYvEEYOfkGycN86htHDghVY58qD8\n\tIlJc1TjONhQykbtVo1skMgnLX9kXpc0N1sS9z7Fe7b3cJ0nZlvBqds1keJcdgicWsDmtS9Toaui\n\tezBHKYBS6bXdVWSZQIWzPPltNVOM8FnqNUqkGkabSkR/AA9ZfTZMAUSlLlna+N/L4Ur3t0RVosl\n\tcrDIvU6+LqLy4fduSv266SzmUFo=","X-Received":["by 2002:a05:600c:3d0d:b0:434:ff08:202e with SMTP id\n\t5b1f17b1804b1-438e15ee1cemr71428755e9.8.1738353625542; \n\tFri, 31 Jan 2025 12:00:25 -0800 (PST)","by 2002:a05:600c:3d0d:b0:434:ff08:202e with SMTP id\n\t5b1f17b1804b1-438e15ee1cemr71428545e9.8.1738353625171; \n\tFri, 31 Jan 2025 12:00:25 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IF2z/pBwUTKDz3fgFt00ahwqFF7OBimgcYJQQeexkiVntJAe4wC1MSV7NacNVLGiSLmSbk3IQ==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStanislaw Gruszka , =?utf-8?q?Barn?=\n\t=?utf-8?b?YWLDoXMgUMWRY3pl?= ","Subject":"Re: [PATCH v2] libcamera: software_isp: Handle signals in the\n\tproper thread","In-Reply-To":"<20250131192945.GJ12673@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Fri, 31 Jan 2025 21:29:45 +0200\")","References":"<20250131191838.47661-1-mzamazal@redhat.com>\n\t<20250131192945.GJ12673@pendragon.ideasonboard.com>","Date":"Fri, 31 Jan 2025 21:00:23 +0100","Message-ID":"<85lduqwy7c.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"l0pnJ-RXqE9A5_zKFmq1YeLRs3mrqSGQGiE75nytzfY_1738353626","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\" "}}]