[{"id":34501,"web_url":"https://patchwork.libcamera.org/comment/34501/","msgid":"<20250616225624.GF28664@pendragon.ideasonboard.com>","date":"2025-06-16T22:56:24","subject":"Re: [PATCH v10 03/13] config: Make GlobalConfiguration instance","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, Jun 16, 2025 at 10:47:21AM +0200, Milan Zamazal wrote:\n> Global configuration is accessed via a GlobalConfiguration instance.\n> The instance is conceptually a singleton, but singletons are not welcome\n> in libcamera so we must store the (preferably single) instance\n> somewhere.\n> \n> This patch creates a GlobalConfiguration instance in CameraManager and\n> defines the corresponding access method. CameraManager is typically\n> instantiated only once or a few times, it is accessible in\n> many places in libcamera and the configuration can be retrieved from it\n> and passed to other places if needed (it's read-only once created).\n> Using CameraManager for the purpose is still suboptimal and we use it\n> only due to lack of better options.\n\nWhy is it sub-optimal ? CameraManager is really the central component in\nlibcamera, so I think it's the natural location to handle the global\nconfiguration file. The fact that the logger is a singleton is something\nI would ideally like to fix.\n\n> An alternative could be Logger,\n> which is still a singleton and it's accessible from everywhere. But\n> with Logger, we have a chicken and egg problem -- GlobalConfiguration\n> should log contingent problems with the configuration when it's loaded\n> but if it is created in the logger then there are mutual infinite\n> recursive calls. Perhaps some acceptable workaround could be found,\n> which would also allow an easier logging configuration.\n> \n> If there are multiple CameraManager instances, there are also multiple\n> GlobalConfiguration instances. They may or may not contain the same\n> data, depending on whether the global configuration file in the file\n> system was changed in the meantime. This is dirty and a potential\n> source of trouble (although not expected to have much impact in\n> practice) but there is no idea how to do better if we want to avoid\n> having GlobalConfiguration singleton or dealing with the logging\n> problem.\n\nI think it would make sense to let applications pass a custom\nconfiguration file to the CameraManager when creating it, so I don't\nreally think this is an issue, or dirty. If multiple instances of\nCameraManager are created, they should be completely independent from\neach other.\n\nThe implementation looks fine. Depending on the outcome of the\ndiscussion in 02/13 about error handling, we may need to propagate\nerrors to applications.\n\n> The configuration is stored in CameraManager privately, it's not meant\n> to be accessed by applications.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> include/libcamera/internal/camera_manager.h | 8 ++++++++\n> src/libcamera/camera_manager.cpp | 8 ++++++++\n> 2 files changed, 16 insertions(+)\n> \n> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\n> index 0150ca61f..e8082eb34 100644\n> --- a/include/libcamera/internal/camera_manager.h\n> +++ b/include/libcamera/internal/camera_manager.h\n> @@ -18,6 +18,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/global_configuration.h\"\n> #include \"libcamera/internal/process.h\"\n> \n> namespace libcamera {\n> @@ -38,6 +39,11 @@ public:\n> \tvoid addCamera(std::shared_ptr camera) LIBCAMERA_TSA_EXCLUDES(mutex_);\n> \tvoid removeCamera(std::shared_ptr camera) LIBCAMERA_TSA_EXCLUDES(mutex_);\n> \n> +\tconst GlobalConfiguration &configuration() const\n> +\t{\n> +\t\treturn configuration_;\n> +\t}\n> +\n> \tIPAManager *ipaManager() const { return ipaManager_.get(); }\n> \n> protected:\n> @@ -66,6 +72,8 @@ private:\n> \n> \tstd::unique_ptr ipaManager_;\n> \tProcessManager processManager_;\n> +\n> +\tconst GlobalConfiguration configuration_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index e62e7193c..f3b4ec708 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -15,6 +15,7 @@\n> \n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> +#include \"libcamera/internal/global_configuration.h\"\n> #include \"libcamera/internal/ipa_manager.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> \n> @@ -254,6 +255,13 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n> \to->cameraRemoved.emit(camera);\n> }\n> \n> +/**\n> + * \\fn const GlobalConfiguration &CameraManager::Private::configuration() const\n> + * \\brief Get global configuration bound to the camera manager\n> + *\n> + * \\return Reference to the configuration\n> + */\n> +\n> /**\n> * \\fn CameraManager::Private::ipaManager() const\n> * \\brief Retrieve the IPAManager","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 5B78CC3237\n\tfor ;\n\tMon, 16 Jun 2025 22:56:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 65B2768DD0;\n\tTue, 17 Jun 2025 00:56:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E324461548\n\tfor ;\n\tTue, 17 Jun 2025 00:56:40 +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 ESMTPSA id B4EE9493;\n\tTue, 17 Jun 2025 00:56:28 +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=\"FbFptCVf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1750114588;\n\tbh=0xSzEIkbPyC1d2uPcZEO8VWZWmFLKWMEHU2se9Ahwn8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FbFptCVfieOQoieDbqpy2pwu4rfAooYG8SvJI0y4WWgprf3uWVRoFWPCI9ZFCqf3Z\n\tDN/ObugLkPC0CHFweyRQFt8PGEEwDJ68qZDV2pcMRachyGKZMqrUJrbQKv3y6lpnMH\n\tbFjokW3odcv4a8lp/J7NsBxvVBO2SVKab27ujb4k=","Date":"Tue, 17 Jun 2025 01:56:24 +0300","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?=\n\t","Subject":"Re: [PATCH v10 03/13] config: Make GlobalConfiguration instance","Message-ID":"<20250616225624.GF28664@pendragon.ideasonboard.com>","References":"<20250616084733.18707-1-mzamazal@redhat.com>\n\t<20250616084733.18707-4-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250616084733.18707-4-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":34589,"web_url":"https://patchwork.libcamera.org/comment/34589/","msgid":"<851pref6im.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-06-20T21:27:29","subject":"Re: [PATCH v10 03/13] config: Make GlobalConfiguration instance","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Mon, Jun 16, 2025 at 10:47:21AM +0200, Milan Zamazal wrote:\n>> Global configuration is accessed via a GlobalConfiguration instance.\n>> The instance is conceptually a singleton, but singletons are not welcome\n>> in libcamera so we must store the (preferably single) instance\n>> somewhere.\n>> \n>> This patch creates a GlobalConfiguration instance in CameraManager and\n>> defines the corresponding access method. CameraManager is typically\n>> instantiated only once or a few times, it is accessible in\n>> many places in libcamera and the configuration can be retrieved from it\n>> and passed to other places if needed (it's read-only once created).\n>> Using CameraManager for the purpose is still suboptimal and we use it\n>> only due to lack of better options.\n>\n> Why is it sub-optimal ? CameraManager is really the central component in\n> libcamera, so I think it's the natural location to handle the global\n> configuration file. \n\nBut this also means that the configuration is not as easily accessible\nas e.g. logging (now). It must be passed to places not having access to\nthe camera manager explicitly, as seen in \"Look up IPA configurables in\nconfiguration file\" patch. But considering your idea of camera manager\nspecific configurations below, this is the way to do it.\n\n> The fact that the logger is a singleton is something I would ideally\n> like to fix.\n\nI'm honestly curious how.\n\n>> An alternative could be Logger,\n>> which is still a singleton and it's accessible from everywhere. But\n>> with Logger, we have a chicken and egg problem -- GlobalConfiguration\n>> should log contingent problems with the configuration when it's loaded\n>> but if it is created in the logger then there are mutual infinite\n>> recursive calls. Perhaps some acceptable workaround could be found,\n>> which would also allow an easier logging configuration.\n>> \n>> If there are multiple CameraManager instances, there are also multiple\n>> GlobalConfiguration instances. They may or may not contain the same\n>> data, depending on whether the global configuration file in the file\n>> system was changed in the meantime. This is dirty and a potential\n>> source of trouble (although not expected to have much impact in\n>> practice) but there is no idea how to do better if we want to avoid\n>> having GlobalConfiguration singleton or dealing with the logging\n>> problem.\n>\n> I think it would make sense to let applications pass a custom\n> configuration file to the CameraManager when creating it, so I don't\n> really think this is an issue, or dirty. If multiple instances of\n> CameraManager are created, they should be completely independent from\n> each other.\n\nOK, makes sense. Except for parts out of CameraManager scope, which is\ncurrently only logging.\n\n> The implementation looks fine. Depending on the outcome of the\n> discussion in 02/13 about error handling, we may need to propagate\n> errors to applications.\n\nI'd like to propagate it but I'm not sure how to do it, as already\nmentioned in my 02/13 response.\n\n>> The configuration is stored in CameraManager privately, it's not meant\n>> to be accessed by applications.\n>> \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> include/libcamera/internal/camera_manager.h | 8 ++++++++\n>> src/libcamera/camera_manager.cpp | 8 ++++++++\n>> 2 files changed, 16 insertions(+)\n>> \n>> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h\n>> index 0150ca61f..e8082eb34 100644\n>> --- a/include/libcamera/internal/camera_manager.h\n>> +++ b/include/libcamera/internal/camera_manager.h\n>> @@ -18,6 +18,7 @@\n>> #include \n>> #include \n>> \n>> +#include \"libcamera/internal/global_configuration.h\"\n>> #include \"libcamera/internal/process.h\"\n>> \n>> namespace libcamera {\n>> @@ -38,6 +39,11 @@ public:\n>> \tvoid addCamera(std::shared_ptr camera) LIBCAMERA_TSA_EXCLUDES(mutex_);\n>> \tvoid removeCamera(std::shared_ptr camera) LIBCAMERA_TSA_EXCLUDES(mutex_);\n>> \n>> +\tconst GlobalConfiguration &configuration() const\n>> +\t{\n>> +\t\treturn configuration_;\n>> +\t}\n>> +\n>> \tIPAManager *ipaManager() const { return ipaManager_.get(); }\n>> \n>> protected:\n>> @@ -66,6 +72,8 @@ private:\n>> \n>> \tstd::unique_ptr ipaManager_;\n>> \tProcessManager processManager_;\n>> +\n>> +\tconst GlobalConfiguration configuration_;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n>> index e62e7193c..f3b4ec708 100644\n>> --- a/src/libcamera/camera_manager.cpp\n>> +++ b/src/libcamera/camera_manager.cpp\n>> @@ -15,6 +15,7 @@\n>> \n>> #include \"libcamera/internal/camera.h\"\n>> #include \"libcamera/internal/device_enumerator.h\"\n>> +#include \"libcamera/internal/global_configuration.h\"\n>> #include \"libcamera/internal/ipa_manager.h\"\n>> #include \"libcamera/internal/pipeline_handler.h\"\n>> \n>> @@ -254,6 +255,13 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n>> \to->cameraRemoved.emit(camera);\n>> }\n>> \n>> +/**\n>> + * \\fn const GlobalConfiguration &CameraManager::Private::configuration() const\n>> + * \\brief Get global configuration bound to the camera manager\n>> + *\n>> + * \\return Reference to the configuration\n>> + */\n>> +\n>> /**\n>> * \\fn CameraManager::Private::ipaManager() const\n>> * \\brief Retrieve the IPAManager","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 DD523C3237\n\tfor ;\n\tFri, 20 Jun 2025 21:27:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2973E68DD0;\n\tFri, 20 Jun 2025 23:27:36 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 08F6561535\n\tfor ;\n\tFri, 20 Jun 2025 23:27:34 +0200 (CEST)","from mail-wr1-f71.google.com (mail-wr1-f71.google.com\n\t[209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-60-FQKSe9BVNguebaTgto8f_A-1; Fri, 20 Jun 2025 17:27:32 -0400","by mail-wr1-f71.google.com with SMTP id\n\tffacd0b85a97d-3a4ff581df3so1163799f8f.1\n\tfor ;\n\tFri, 20 Jun 2025 14:27:32 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-3a6d1190b00sm2914723f8f.87.2025.06.20.14.27.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 20 Jun 2025 14:27:30 -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=\"HaXqVRyx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1750454854;\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=VZpVSMSyyeXZS77/FUFY0kV0tl6EIz0q+5wW8E5xRlk=;\n\tb=HaXqVRyxeZbwQuvY2YpO8/W1FhnJMCTtYpqbfUcektawgrDCM5y1xTNt9oexsEyQlLZqFP\n\tpRj4/78PYuruSWx7JSTvITp0vBUCjy37B/EqcQpzdYcNlDa2w2mMjZ4jzVN9V68jsQ/+6L\n\tXrQPtm1rcnOkB4PpQyq2Cc44Mk2r++k=","X-MC-Unique":"FQKSe9BVNguebaTgto8f_A-1","X-Mimecast-MFC-AGG-ID":"FQKSe9BVNguebaTgto8f_A_1750454851","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1750454851; x=1751059651;\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=VZpVSMSyyeXZS77/FUFY0kV0tl6EIz0q+5wW8E5xRlk=;\n\tb=fbXoEJhtQDzpgBbxbRmePZI9OoHZBGuVe/8g9d8fPZduSv/XiAaG16RUF850SrG1kQ\n\tzTYxRH/LUPm2X+kPHR5ev7Vd+nsF0qSma3h8Sr1L+A6JKpZb8elTEJZDZ4XB0F5eKUx+\n\talOWXRaI06DTD9iSORROPw77douFrWIkgFbvuhH8h96KzmEvM682+FopPmk4z8vJ/nxu\n\tt+XUESeTFuBHP35BivMZNTwT763NX9xOIpdhOoUpagC/VvxzuFYn5rsmU2+hCAO/pFfy\n\tSQBVZwZ3ypv9uYL3AJu1scGbXw55t5dAhrry722FZo6rymcXGZUsjHGiwyCVmWzeDGfI\n\tB82A==","X-Gm-Message-State":"AOJu0YznBc4lIPpNUcJeWKwwH5Vyd2WH015XDn1zo57QBPAvNc1Vv/WI\n\tFJYJ/fS5KXq5emGyfM0I75hwkFcmC/Lon9ut2w7vNqXso3jQrS69daDJZfi7WbJt63TNwbD78FS\n\t5CJ8a6xcR1sJww+tYt4oWzbFd7wb+DHmZ6fiPBomOtDMFNHsAqaY4vrWxDftUJjppiyokwcMpeP\n\tQ=","X-Gm-Gg":"ASbGncuwyVnVEGREQtwUI7fDvN8VZrE45tRlFmF/Zgl+1P+imxDzzIFp6fqw2X6crlY\n\tcmOPAhUFgd2kyLua5jc0RBBl7hlkv/NiYWK+Fhl2KppDaC4a1Xkt0fD9rep2VmLSRTZYnhIbY3H\n\tZzCXFD4UElKn5Eoo/h+5JcikhxslPyM2Ig9zt1Fh9NPdDxy2Wy6DIk6pGCGZUVI0v1a7q4/rqjD\n\tbk3akPqtUDqC6/oiUHK7y7gkS2yqCvio96al50W2PpnPADKzMA/QSr3jnVeoCgjaZ8nG3dKFlmy\n\tLeG6OS6XcuM6yjZ7oA8jJmOUL+P24VJg95TsDAvICk5iZCvvVbAyxliomdHNXP8G8LFRy4PD7cw\n\t=","X-Received":["by 2002:a05:6000:5c2:b0:3a5:298a:3214 with SMTP id\n\tffacd0b85a97d-3a6d27cf386mr3519139f8f.13.1750454851293; \n\tFri, 20 Jun 2025 14:27:31 -0700 (PDT)","by 2002:a05:6000:5c2:b0:3a5:298a:3214 with SMTP id\n\tffacd0b85a97d-3a6d27cf386mr3519133f8f.13.1750454850898; \n\tFri, 20 Jun 2025 14:27:30 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IH/SsW4Z1nHcqijtwDIvSzKj8AtHDY3/4A/uanVrlrEGNhrDTTaZ1qHubvDhNqYj2hInRuHfQ==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham , =?utf-8?q?Barnab?=\n\t=?utf-8?b?w6FzIFDFkWN6ZQ==?= ","Subject":"Re: [PATCH v10 03/13] config: Make GlobalConfiguration instance","In-Reply-To":"<20250616225624.GF28664@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Tue, 17 Jun 2025 01:56:24 +0300\")","References":"<20250616084733.18707-1-mzamazal@redhat.com>\n\t<20250616084733.18707-4-mzamazal@redhat.com>\n\t<20250616225624.GF28664@pendragon.ideasonboard.com>","Date":"Fri, 20 Jun 2025 23:27:29 +0200","Message-ID":"<851pref6im.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"KF-Hu9_Xwq51JGryB1HDKbM6_tpt4LU9w3YQOkiVnlQ_1750454851","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\" "}}]