[{"id":37644,"web_url":"https://patchwork.libcamera.org/comment/37644/","msgid":"","date":"2026-01-14T10:28:29","subject":"Re: [PATCH 20/36] libcamera: Pass CameraManager around instead of\n\tGlobalConfiguration","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2026. 01. 13. 1:07 keltezéssel, Laurent Pinchart írta:\n> The GlobalConfiguration is explicitly passed around through constructors\n> of various objects that need access to the configuration. This ad-hoc\n> solution works for the specific use cases it was meant to support, but\n> isn't very generic. We have a top-level object in libcamera, the\n> CameraManager, that also needs to be accessed from various locations and\n> is passed to object constructors. Standardize on passing the\n> CameraManager everywhere, and access the global configuration through\n> it.\n\nWhen the `GlobalConfiguration` was originally proposed, I was concerned that\npassing the whole `GlobalConfiguration` object to e.g. the `Benchmark` class\nis likely a suboptimal decision because not only does it only need two integers,\npassing the configuration means that it has to have knowledge of the entire\npaths of these integers: limiting reusability greatly. I am still concerned,\nand the same applies to `CameraManager` (if not more so). But I don't think\nthat changes the status quo significantly, so it is probably fine, but it will\nmake `CameraManager` a real god object now.\n\n\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/ipa_manager.h | 10 +++++-----\n> include/libcamera/internal/ipa_proxy.h | 4 ++--\n> .../libcamera/internal/software_isp/benchmark.h | 6 +++---\n> .../internal/software_isp/swstats_cpu.h | 6 +++---\n> src/libcamera/camera_manager.cpp | 3 ++-\n> src/libcamera/ipa_manager.cpp | 7 +++++--\n> src/libcamera/ipa_proxy.cpp | 16 +++++++++++-----\n> src/libcamera/software_isp/benchmark.cpp | 7 ++++++-\n> src/libcamera/software_isp/debayer.cpp | 13 ++++++-------\n> src/libcamera/software_isp/debayer.h | 4 ++--\n> src/libcamera/software_isp/debayer_cpu.cpp | 7 ++++---\n> src/libcamera/software_isp/debayer_cpu.h | 3 ++-\n> src/libcamera/software_isp/debayer_egl.cpp | 7 +++----\n> src/libcamera/software_isp/debayer_egl.h | 4 +++-\n> src/libcamera/software_isp/software_isp.cpp | 9 +++++----\n> src/libcamera/software_isp/swstats_cpu.cpp | 8 ++++----\n> test/ipa/ipa_interface_test.cpp | 5 +----\n> .../module_ipa_proxy.cpp.tmpl | 8 ++++----\n> .../libcamera_templates/module_ipa_proxy.h.tmpl | 4 ++--\n> 19 files changed, 73 insertions(+), 58 deletions(-)\n> \n> [...]\n> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp\n> index c1fe2267cc6e..271c4e2c9dc2 100644\n> --- a/test/ipa/ipa_interface_test.cpp\n> +++ b/test/ipa/ipa_interface_test.cpp\n> @@ -47,7 +47,6 @@ public:\n> \t\tnotifier_.reset();\n> \t\tipa_.reset();\n> \t\tipaManager_.reset();\n> -\t\tconfig_.reset();\n> \t\tcameraManager_.reset();\n> \t}\n> \n> @@ -90,8 +89,7 @@ protected:\n> \t\tnotifier_->activated.connect(this, &IPAInterfaceTest::readTrace);\n> \n> \t\t/* Create the IPA manager. */\n> -\t\tconfig_ = std::make_unique();\n> -\t\tipaManager_ = std::make_unique(*config_);\n> +\t\tipaManager_ = std::make_unique(*cameraManager_);\n\nI'm wondering, does this make it possible for configuration in e.g. the user's home\ndirectory to influence the tests? And I realize that that was still the case before\nthis patch series, but would still be nice to have a way to limit the effect.\nI suppose we want to add the config file location env vars and override them\nin the tests to ensure consistent behaviour.\n\n\nReviewed-by: Barnabás Pőcze \n\n\n> \n> \t\treturn TestPass;\n> \t}\n> @@ -169,7 +167,6 @@ private:\n> \tstd::shared_ptr pipe_;\n> \tstd::unique_ptr ipa_;\n> \tstd::unique_ptr cameraManager_;\n> -\tstd::unique_ptr config_;\n> \tstd::unique_ptr ipaManager_;\n> \tenum ipa::vimc::IPAOperationCode trace_;\n> \tstd::unique_ptr notifier_;","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 11709C3226\n\tfor ;\n\tWed, 14 Jan 2026 10:28:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 553D861FA3;\n\tWed, 14 Jan 2026 11:28:35 +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 5BBF4615B2\n\tfor ;\n\tWed, 14 Jan 2026 11:28:33 +0100 (CET)","from [192.168.33.18] (185.221.143.114.nat.pool.zt.hu\n\t[185.221.143.114])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 76D6B4BB;\n\tWed, 14 Jan 2026 11:28:06 +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=\"Rw8/DTi3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768386486;\n\tbh=0iiaPvErq6mAFUJfQrGY8vG/ocecwZHkphhMZv1T/pk=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=Rw8/DTi323ZB8kxpexBHgK3TmZvn9npcLG+8u0M6Ww59uVl56ryaZN7wXy4fpibaN\n\tV7MsVSc9uG47jYEE0UQO9Ykj+0df2cIPz3iNwFc2C5ftHnUTZf6RHBCMEox3OBRswL\n\tC4wwyvxA8WzrpDlEiRYMb217JmJaBN1ekESb3qWs=","Message-ID":"","Date":"Wed, 14 Jan 2026 11:28:29 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 20/36] libcamera: Pass CameraManager around instead of\n\tGlobalConfiguration","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20260113000808.15395-1-laurent.pinchart@ideasonboard.com>\n\t<20260113000808.15395-21-laurent.pinchart@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20260113000808.15395-21-laurent.pinchart@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":37712,"web_url":"https://patchwork.libcamera.org/comment/37712/","msgid":"<20260118225608.GC7759@pendragon.ideasonboard.com>","date":"2026-01-18T22:56:08","subject":"Re: [PATCH 20/36] libcamera: Pass CameraManager around instead of\n\tGlobalConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jan 14, 2026 at 11:28:29AM +0100, Barnabás Pőcze wrote:\n> 2026. 01. 13. 1:07 keltezéssel, Laurent Pinchart írta:\n> > The GlobalConfiguration is explicitly passed around through constructors\n> > of various objects that need access to the configuration. This ad-hoc\n> > solution works for the specific use cases it was meant to support, but\n> > isn't very generic. We have a top-level object in libcamera, the\n> > CameraManager, that also needs to be accessed from various locations and\n> > is passed to object constructors. Standardize on passing the\n> > CameraManager everywhere, and access the global configuration through\n> > it.\n> \n> When the `GlobalConfiguration` was originally proposed, I was concerned that\n> passing the whole `GlobalConfiguration` object to e.g. the `Benchmark` class\n> is likely a suboptimal decision because not only does it only need two integers,\n> passing the configuration means that it has to have knowledge of the entire\n> paths of these integers: limiting reusability greatly. I am still concerned,\n> and the same applies to `CameraManager` (if not more so). But I don't think\n> that changes the status quo significantly, so it is probably fine, but it will\n> make `CameraManager` a real god object now.\n\nI share the same concern. It could be nicer if objects didn't have to\nencode the full path to their configuration data, but I haven't found a\nway to achieve that that I'm fully happy with.\n\nI would like to further rework configuration handling to provide\nmultiple levels in the configuration file: options that apply to cameras\nshould be specified per camera, with the ability to set a default for\nthe pipeline handler. I'm envisioning an API where the pipeline handler\ncould request configuration data for a camera (to the CameraManager, the\nGlobalConfiguration or the Camera class, not sure yet), and that data\nwould be constructed from the various means of specifying configuration\noptions (environment variables, pipeline handler level configuration in\nthe configuration file, camera level configuration in the configuration\nfile, and configuration data passed to the CameraManager constructor -\nor possibly start() function). I haven't worked on that yet, and I\nexpect further rework to the way the configuration is handled\ninternally, but I still think that this patch goes in the right\ndirection for the time being.\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/ipa_manager.h | 10 +++++-----\n> > include/libcamera/internal/ipa_proxy.h | 4 ++--\n> > .../libcamera/internal/software_isp/benchmark.h | 6 +++---\n> > .../internal/software_isp/swstats_cpu.h | 6 +++---\n> > src/libcamera/camera_manager.cpp | 3 ++-\n> > src/libcamera/ipa_manager.cpp | 7 +++++--\n> > src/libcamera/ipa_proxy.cpp | 16 +++++++++++-----\n> > src/libcamera/software_isp/benchmark.cpp | 7 ++++++-\n> > src/libcamera/software_isp/debayer.cpp | 13 ++++++-------\n> > src/libcamera/software_isp/debayer.h | 4 ++--\n> > src/libcamera/software_isp/debayer_cpu.cpp | 7 ++++---\n> > src/libcamera/software_isp/debayer_cpu.h | 3 ++-\n> > src/libcamera/software_isp/debayer_egl.cpp | 7 +++----\n> > src/libcamera/software_isp/debayer_egl.h | 4 +++-\n> > src/libcamera/software_isp/software_isp.cpp | 9 +++++----\n> > src/libcamera/software_isp/swstats_cpu.cpp | 8 ++++----\n> > test/ipa/ipa_interface_test.cpp | 5 +----\n> > .../module_ipa_proxy.cpp.tmpl | 8 ++++----\n> > .../libcamera_templates/module_ipa_proxy.h.tmpl | 4 ++--\n> > 19 files changed, 73 insertions(+), 58 deletions(-)\n> > \n> > [...]\n> > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp\n> > index c1fe2267cc6e..271c4e2c9dc2 100644\n> > --- a/test/ipa/ipa_interface_test.cpp\n> > +++ b/test/ipa/ipa_interface_test.cpp\n> > @@ -47,7 +47,6 @@ public:\n> > \t\tnotifier_.reset();\n> > \t\tipa_.reset();\n> > \t\tipaManager_.reset();\n> > -\t\tconfig_.reset();\n> > \t\tcameraManager_.reset();\n> > \t}\n> > \n> > @@ -90,8 +89,7 @@ protected:\n> > \t\tnotifier_->activated.connect(this, &IPAInterfaceTest::readTrace);\n> > \n> > \t\t/* Create the IPA manager. */\n> > -\t\tconfig_ = std::make_unique();\n> > -\t\tipaManager_ = std::make_unique(*config_);\n> > +\t\tipaManager_ = std::make_unique(*cameraManager_);\n> \n> I'm wondering, does this make it possible for configuration in e.g. the user's home\n> directory to influence the tests? And I realize that that was still the case before\n> this patch series, but would still be nice to have a way to limit the effect.\n> I suppose we want to add the config file location env vars and override them\n> in the tests to ensure consistent behaviour.\n\nThat's a good question. I can imagine pros and cons, and it may even\ndepend on which configuration option we're talking about. I don't have\nan answer yet.\n\n> Reviewed-by: Barnabás Pőcze \n> \n> > \n> > \t\treturn TestPass;\n> > \t}\n> > @@ -169,7 +167,6 @@ private:\n> > \tstd::shared_ptr pipe_;\n> > \tstd::unique_ptr ipa_;\n> > \tstd::unique_ptr cameraManager_;\n> > -\tstd::unique_ptr config_;\n> > \tstd::unique_ptr ipaManager_;\n> > \tenum ipa::vimc::IPAOperationCode trace_;\n> > \tstd::unique_ptr notifier_;","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 C6973BDCBF\n\tfor ;\n\tSun, 18 Jan 2026 22:56:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F4FB61FC3;\n\tSun, 18 Jan 2026 23:56:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 557EA61FB7\n\tfor ;\n\tSun, 18 Jan 2026 23:56:30 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-152.bb.dnainternet.fi\n\t[81.175.209.152])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 163D622A;\n\tSun, 18 Jan 2026 23:55:59 +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=\"HfICqE0y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768776960;\n\tbh=TIIFbf6ZvPABXv9Bfk+Z/PQJv4yzC2kQaF6uPk4Fr1s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HfICqE0yZ7kxXNiVoVUAlb3D9AEfJYeBx80SLI+7RzFALgpOb80ofJF3j6KsRdZqZ\n\tz/McjRgtKv4RdHqPC1qDGueMnyqgaDrOQrj4WMWGCWovWAshwcDxaHOBwe7tIodLkc\n\teV8IcGRdoKdAc4CTaLUXisQd+cn3OLYIKNJDGKMk=","Date":"Mon, 19 Jan 2026 00:56:08 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 20/36] libcamera: Pass CameraManager around instead of\n\tGlobalConfiguration","Message-ID":"<20260118225608.GC7759@pendragon.ideasonboard.com>","References":"<20260113000808.15395-1-laurent.pinchart@ideasonboard.com>\n\t<20260113000808.15395-21-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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\" "}}]