[{"id":34091,"web_url":"https://patchwork.libcamera.org/comment/34091/","msgid":"","date":"2025-05-01T11:15:39","subject":"Re: [PATCH v8 00/15] Add global configuration file","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 04. 30. 14:14 keltezéssel, Milan Zamazal írta:\n> This patch series introduces global configuration file for libcamera, to\n> provide runtime configuration means other than environment variables.\n> Instead of, or in addition to, the growing list of configuration\n> environment variables, the whole configuration can be specified in a\n> single configuration file. This is both simpler and more flexible.\n> \n> This is not a replacement for specific configuration files already\n> present in libcamera.\n> \n> The patches implement what is needed to introduce a configuration file\n> that can handle the current environment variables and software ISP\n> TODOs. They demonstrate how to deal with the key points that must be\n> considered. See commit messages for more details.\n> \n> The configuration file is a YAML file. It is looked up in user’s home\n> directory or, if not present, in system-wide libcamera directories.\n> Both the directories and file name of the configuration file can be\n> overridden using newly introduced environment variables.\n> \n> The current configuration environment variables are still supported and\n> take precedence, if defined, over the corresponding entries in the YAML\n> file.\n> \n> This patch series is not exhaustive, there can be future enhancements,\n> most notably configuration file validation to avoid confusions caused by\n> typos etc.\n> \n> Not everything has been tested because some of the patches are related\n> to specific hardware.\n> \n> Also, not everything is necessarily C++ elegant, suggestions for\n> improvements are welcome. Note that there is an obstacle in that\n> configuration initialization may call logging and logging queries\n> configuration, as explained in the commit messages. This complicates\n> transparent encapsulation.\n\nI think generally the direction is to eliminate singletons from libcamera.[0][1]\nSo I am not sure that a new one should be introduced. I believe we should proceed\ntowards a design where the configuration is per-CameraManager (e.g. the constructor\nloads the configuration), and then it is propagated to other components. (As far as\nI can see, this shouldn't be too much trouble: pipeline handlers can reach the CameraManager\neasily, and `DebayerCpu` can receive the settings in its constructor via `SoftwareISP`\nfrom the simple pipeline handler, `IPAManager` can receive it in its constructor).\n\nSecondly, I would consider dropping the logging related settings from the configuration\nfile, at least for now. It is likely not the most interesting part, and even with the\nsingleton design, I am not fan of the quite fragile `GlobalConfiguration::initialize()`\ncall in the `IPAManager` constructor. (With per-CameraManager configuration, I could\nimagine that the constructor initializes the logger based on the configuration. Of\ncourse if two CameraManagers load different configuration, then it is entirely possible\nthat they will overwrite each other's log settings, but unless we want to inject\nseparate logger objects into each object of interest, I don't really see a nice\nsolution; but it's probably not a big issue anyways.)\n\nThoughts?\n\n[0]: https://bugs.libcamera.org/show_bug.cgi?id=246\n[1]: https://lists.libcamera.org/pipermail/libcamera-devel/2025-April/049888.html\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Changes in v8:\n> - Rebased on current master.\n> - Anniversary edition: 400 days since v1 posted. ;-)\n> \n> Changes in v7:\n> - Rebased on current master.\n> - Tuning file path configuration updated for recent changes.\n> A significant change is that the tuning file configuration is no\n> longer sensor dependent as there is apparently no access to the sensor\n> info in the IPA proxy.\n> - Minor improvements of some commit messages.\n> \n> Changes in v6:\n> - Rebased on master.\n> - File names from file header descriptions removed.\n> - Indentation fix in moved code as requested by checkstyle.py.\n> - Unneeded const_cast's removed.\n> - Using GlobalConfiguration namespace rather than a class.\n> - Path configuration options are defined as sequences in YAML.\n> - A patch introducing LIBCAMERA_CONFIG_NAME added.\n> - A patch introducing LIBCAMERA_CONFIG_DIR added.\n> - Miscellaneous minor code changes suggested by Barnabás.\n> \n> Changes in v5:\n> - A pointer is used to store the global configuration singleton rather\n> than a static variable. This makes the things more robust and fixes\n> the problem with re-entrancy on logging and a failing\n> camera_reconfigure test.\n> - In relation to the change above, a new initialization method\n> GlobalConfiguration::initialize() was introduced that replaces the\n> initialization calls in CameraManager and IPAManager.\n> - Logging YAML errors when reading the configuration was also fixed.\n> - Global configuration is placed to base directly, without an\n> intermediate patch.\n> - An `optional' value comparison simplified.\n> - A temporary typo in a comment fixed.\n> - Unused stdint.h include removed.\n> \n> Changes in v4:\n> - Rebased on current master.\n> - Configuration option for LIBCAMERA_IPA_PROXY_PATH added.\n> - Added a patch to include stdlib.h instead of cstdlib in yaml_parser.cpp.\n> \n> Changes in v3:\n> - Added a configuration item for the newly introduced\n> LIBCAMERA_PIPELINES_MATCH_LIST variable.\n> - A minor indentation fix.\n> - Fixed `pipelines.' x `pipeline.' configuration item naming mismatch.\n> - Tuning files are looked up now by particular cameras attached rather than\n> being specified for the whole pipeline.\n> - Helpers use std::string& instead of char* for confPath arguments.\n> - Protection against returning YamlObject::empty as a regular value (the\n> problem has been probably exposed by addition of\n> LIBCAMERA_PIPELINES_MATCH_LIST).\n> \n> Changes in v2:\n> - Rebased on master.\n> - Various cleanups, documentation improvements and minor fixes.\n> - Configuration option for LIBCAMERA_RPI_TUNING_FILE added (Naush).\n> - Two more patches for software ISP configuration added.\n> \n> Milan Zamazal (15):\n> yaml: Move yaml_parser.cpp to base\n> config: Introduce global runtime configuration\n> config: Look up logging levels in the configuration file\n> config: Add configuration retrieval helpers\n> config: Look up log file in configuration file\n> config: Look up log color configuration in configuration file\n> config: Look up rpi config path in configuration file\n> config: Look up IPA configurables in configuration file\n> config: Look up pipelines match list in configuration file\n> config: Allow enabling software ISP in runtime\n> config: Add global configuration file documentation\n> libcamera: software_isp: Make input buffer copying configurable\n> libcamera: software_isp: Make measurement configurable\n> config: Make configuration file configurable\n> config: Make configuration directories configurable\n> \n> Documentation/documentation-contents.rst | 2 +-\n> Documentation/index.rst | 2 +-\n> Documentation/meson.build | 2 +-\n> ...ariables.rst => runtime_configuration.rst} | 133 +++++++-\n> .../libcamera/internal/global_configuration.h | 60 ++++\n> include/libcamera/internal/meson.build | 1 +\n> src/libcamera/base/global_configuration.cpp | 289 ++++++++++++++++++\n> src/libcamera/base/log.cpp | 40 +--\n> src/libcamera/base/meson.build | 15 +\n> src/libcamera/{ => base}/yaml_parser.cpp | 13 +-\n> src/libcamera/camera_manager.cpp | 10 +-\n> src/libcamera/ipa_manager.cpp | 35 ++-\n> src/libcamera/ipa_proxy.cpp | 65 ++--\n> src/libcamera/meson.build | 14 -\n> .../pipeline/rpi/common/pipeline_base.cpp | 8 +-\n> src/libcamera/pipeline/simple/simple.cpp | 12 +\n> src/libcamera/process.cpp | 11 +-\n> src/libcamera/software_isp/TODO | 36 ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 30 +-\n> src/libcamera/software_isp/debayer_cpu.h | 7 +-\n> 20 files changed, 630 insertions(+), 155 deletions(-)\n> rename Documentation/{environment_variables.rst => runtime_configuration.rst} (58%)\n> create mode 100644 include/libcamera/internal/global_configuration.h\n> create mode 100644 src/libcamera/base/global_configuration.cpp\n> rename src/libcamera/{ => base}/yaml_parser.cpp (98%)\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 CC256BE08B\n\tfor ;\n\tThu, 1 May 2025 11:15:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC63B68AD9;\n\tThu, 1 May 2025 13:15:44 +0200 (CEST)","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 12E2D617D7\n\tfor ;\n\tThu, 1 May 2025 13:15:44 +0200 (CEST)","from [192.168.33.21] (185.221.143.50.nat.pool.zt.hu\n\t[185.221.143.50])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9F2DD2E0;\n\tThu, 1 May 2025 13:15:36 +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=\"NDSh8Uwg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746098136;\n\tbh=PFsB4ZS7/TvP44EN9KoP5IqJjCx12r5BJ6TSI0f7IeI=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=NDSh8Uwgbw46q0R1c/UkpKlmC68vmZTDP7RM+BCATf3bFPnXQkRAUwQse8sGRat/o\n\tFduyuweQchuVcugO9Z6A9aeJBsLBsblyIE1lMjcgMUaZ14sc26ta1CoZlq464ZtcDR\n\tTerpdmw4uq5+fz2LCIBEJxaiymj318aOjxzxNW/E=","Message-ID":"","Date":"Thu, 1 May 2025 13:15:39 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v8 00/15] Add global configuration file","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Cc":"Kieran Bingham ,\n\tLaurent Pinchart ","References":"<20250430121449.39910-1-mzamazal@redhat.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250430121449.39910-1-mzamazal@redhat.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":34206,"web_url":"https://patchwork.libcamera.org/comment/34206/","msgid":"<858qn1ptj0.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-05-12T18:34:43","subject":"Re: [PATCH v8 00/15] Add global configuration file","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Barnabás,\n\nBarnabás Pőcze writes:\n\n> Hi\n>\n>\n> 2025. 04. 30. 14:14 keltezéssel, Milan Zamazal írta:\n>> This patch series introduces global configuration file for libcamera, to\n>> provide runtime configuration means other than environment variables.\n>> Instead of, or in addition to, the growing list of configuration\n>> environment variables, the whole configuration can be specified in a\n>> single configuration file. This is both simpler and more flexible.\n>> This is not a replacement for specific configuration files already\n>> present in libcamera.\n>> The patches implement what is needed to introduce a configuration file\n>> that can handle the current environment variables and software ISP\n>> TODOs. They demonstrate how to deal with the key points that must be\n>> considered. See commit messages for more details.\n>> The configuration file is a YAML file. It is looked up in user’s home\n>> directory or, if not present, in system-wide libcamera directories.\n>> Both the directories and file name of the configuration file can be\n>> overridden using newly introduced environment variables.\n>> The current configuration environment variables are still supported and\n>> take precedence, if defined, over the corresponding entries in the YAML\n>> file.\n>> This patch series is not exhaustive, there can be future enhancements,\n>> most notably configuration file validation to avoid confusions caused by\n>> typos etc.\n>> Not everything has been tested because some of the patches are related\n>> to specific hardware.\n>> Also, not everything is necessarily C++ elegant, suggestions for\n>> improvements are welcome. Note that there is an obstacle in that\n>> configuration initialization may call logging and logging queries\n>> configuration, as explained in the commit messages. This complicates\n>> transparent encapsulation.\n>\n> I think generally the direction is to eliminate singletons from libcamera.[0][1]\n> So I am not sure that a new one should be introduced. I believe we should proceed\n> towards a design where the configuration is per-CameraManager (e.g. the constructor\n> loads the configuration), and then it is propagated to other components. (As far as\n> I can see, this shouldn't be too much trouble: pipeline handlers can reach the CameraManager\n> easily, and `DebayerCpu` can receive the settings in its constructor via `SoftwareISP`\n> from the simple pipeline handler, `IPAManager` can receive it in its constructor).\n\nI'm not sure we want the configuration being bound to CameraManager; the\nconfiguration can contain anything and logging may be just one example\nof something more global. I'd prefer to avoid reworking the\nconfiguration mechanism in future.\n\n> Secondly, I would consider dropping the logging related settings from the configuration\n> file, at least for now. It is likely not the most interesting part, \n\nI think logging belongs to the configuration and is a good thing to\nthink about as a proof of the design. Even if we possibly eventually\ndecide to omit it due to technical difficulties, we should think about\nthe overall design more first and have a good answer to the question\n\"why other software can configure logging in its configuration file and\nlibcamera not?\".\n\n> and even with the singleton design, I am not fan of the quite fragile\n> `GlobalConfiguration::initialize()` call in the `IPAManager`\n> constructor.\n\nI suppose nobody is but the question is how to do better without\nimposing artificial restrictions on the configuration and/or introducing\nanother ugliness.\n\n> (With per-CameraManager configuration, I could\n> imagine that the constructor initializes the logger based on the configuration. Of\n> course if two CameraManagers load different configuration, then it is entirely possible\n> that they will overwrite each other's log settings, but unless we want to inject\n> separate logger objects into each object of interest, I don't really see a nice\n> solution; but it's probably not a big issue anyways.)\n\nFrom the practical point of view, the problem of different\nconfigurations is indeed unlikely to be a big issue, the configuration\nwould change only if the user edited it. But a design raising such\nquestions smells and sounds like a call for trouble (configuration\nchanging in runtime unexpectedly, possibly in incompatible/unexpected\nways, ...), I'd rather avoid it.\n\n> Thoughts?\n>\n> [0]: https://bugs.libcamera.org/show_bug.cgi?id=246\n> [1]: https://lists.libcamera.org/pipermail/libcamera-devel/2025-April/049888.html\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n>> Changes in v8:\n>> - Rebased on current master.\n>> - Anniversary edition: 400 days since v1 posted. ;-)\n>> Changes in v7:\n>> - Rebased on current master.\n>> - Tuning file path configuration updated for recent changes.\n>> A significant change is that the tuning file configuration is no\n>> longer sensor dependent as there is apparently no access to the sensor\n>> info in the IPA proxy.\n>> - Minor improvements of some commit messages.\n>> Changes in v6:\n>> - Rebased on master.\n>> - File names from file header descriptions removed.\n>> - Indentation fix in moved code as requested by checkstyle.py.\n>> - Unneeded const_cast's removed.\n>> - Using GlobalConfiguration namespace rather than a class.\n>> - Path configuration options are defined as sequences in YAML.\n>> - A patch introducing LIBCAMERA_CONFIG_NAME added.\n>> - A patch introducing LIBCAMERA_CONFIG_DIR added.\n>> - Miscellaneous minor code changes suggested by Barnabás.\n>> Changes in v5:\n>> - A pointer is used to store the global configuration singleton rather\n>> than a static variable. This makes the things more robust and fixes\n>> the problem with re-entrancy on logging and a failing\n>> camera_reconfigure test.\n>> - In relation to the change above, a new initialization method\n>> GlobalConfiguration::initialize() was introduced that replaces the\n>> initialization calls in CameraManager and IPAManager.\n>> - Logging YAML errors when reading the configuration was also fixed.\n>> - Global configuration is placed to base directly, without an\n>> intermediate patch.\n>> - An `optional' value comparison simplified.\n>> - A temporary typo in a comment fixed.\n>> - Unused stdint.h include removed.\n>> Changes in v4:\n>> - Rebased on current master.\n>> - Configuration option for LIBCAMERA_IPA_PROXY_PATH added.\n>> - Added a patch to include stdlib.h instead of cstdlib in yaml_parser.cpp.\n>> Changes in v3:\n>> - Added a configuration item for the newly introduced\n>> LIBCAMERA_PIPELINES_MATCH_LIST variable.\n>> - A minor indentation fix.\n>> - Fixed `pipelines.' x `pipeline.' configuration item naming mismatch.\n>> - Tuning files are looked up now by particular cameras attached rather than\n>> being specified for the whole pipeline.\n>> - Helpers use std::string& instead of char* for confPath arguments.\n>> - Protection against returning YamlObject::empty as a regular value (the\n>> problem has been probably exposed by addition of\n>> LIBCAMERA_PIPELINES_MATCH_LIST).\n>> Changes in v2:\n>> - Rebased on master.\n>> - Various cleanups, documentation improvements and minor fixes.\n>> - Configuration option for LIBCAMERA_RPI_TUNING_FILE added (Naush).\n>> - Two more patches for software ISP configuration added.\n>> Milan Zamazal (15):\n>> yaml: Move yaml_parser.cpp to base\n>> config: Introduce global runtime configuration\n>> config: Look up logging levels in the configuration file\n>> config: Add configuration retrieval helpers\n>> config: Look up log file in configuration file\n>> config: Look up log color configuration in configuration file\n>> config: Look up rpi config path in configuration file\n>> config: Look up IPA configurables in configuration file\n>> config: Look up pipelines match list in configuration file\n>> config: Allow enabling software ISP in runtime\n>> config: Add global configuration file documentation\n>> libcamera: software_isp: Make input buffer copying configurable\n>> libcamera: software_isp: Make measurement configurable\n>> config: Make configuration file configurable\n>> config: Make configuration directories configurable\n>> Documentation/documentation-contents.rst | 2 +-\n>> Documentation/index.rst | 2 +-\n>> Documentation/meson.build | 2 +-\n>> ...ariables.rst => runtime_configuration.rst} | 133 +++++++-\n>> .../libcamera/internal/global_configuration.h | 60 ++++\n>> include/libcamera/internal/meson.build | 1 +\n>> src/libcamera/base/global_configuration.cpp | 289 ++++++++++++++++++\n>> src/libcamera/base/log.cpp | 40 +--\n>> src/libcamera/base/meson.build | 15 +\n>> src/libcamera/{ => base}/yaml_parser.cpp | 13 +-\n>> src/libcamera/camera_manager.cpp | 10 +-\n>> src/libcamera/ipa_manager.cpp | 35 ++-\n>> src/libcamera/ipa_proxy.cpp | 65 ++--\n>> src/libcamera/meson.build | 14 -\n>> .../pipeline/rpi/common/pipeline_base.cpp | 8 +-\n>> src/libcamera/pipeline/simple/simple.cpp | 12 +\n>> src/libcamera/process.cpp | 11 +-\n>> src/libcamera/software_isp/TODO | 36 ---\n>> src/libcamera/software_isp/debayer_cpu.cpp | 30 +-\n>> src/libcamera/software_isp/debayer_cpu.h | 7 +-\n>> 20 files changed, 630 insertions(+), 155 deletions(-)\n>> rename Documentation/{environment_variables.rst => runtime_configuration.rst} (58%)\n>> create mode 100644 include/libcamera/internal/global_configuration.h\n>> create mode 100644 src/libcamera/base/global_configuration.cpp\n>> rename src/libcamera/{ => base}/yaml_parser.cpp (98%)\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 B342BC3226\n\tfor ;\n\tMon, 12 May 2025 18:34:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD98B614E3;\n\tMon, 12 May 2025 20:34:50 +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 319DF614E3\n\tfor ;\n\tMon, 12 May 2025 20:34:49 +0200 (CEST)","from mail-ej1-f71.google.com (mail-ej1-f71.google.com\n\t[209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-207-v9rp1wX1NoalBexJnLn0EQ-1; Mon, 12 May 2025 14:34:46 -0400","by mail-ej1-f71.google.com with SMTP id\n\ta640c23a62f3a-ad22c5408e7so278510766b.3\n\tfor ;\n\tMon, 12 May 2025 11:34:46 -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\ta640c23a62f3a-ad219746cf4sm658808766b.102.2025.05.12.11.34.43\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 12 May 2025 11:34:43 -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=\"Dw2oj4Dc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1747074888;\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=7MVSZBvPg0Jjqgk4TlVZQWPrkeEuMjE3ZLPGH3PdE5A=;\n\tb=Dw2oj4DcqJU1ESmFZOHXvE6VdYOZfMQ2h2yePsc+0QQEycRM7iTb9BuDhdwy+2k3NaDVDG\n\tEjBIvZ5RABWmRhTH/oMjWmYZ/XjN28zGqkdi+BQq35PnLkwZaYMzKwaWbkobZfVA/46JEP\n\td63EIHh/mi7OEa2aF0vm8CgIC6xQVKQ=","X-MC-Unique":"v9rp1wX1NoalBexJnLn0EQ-1","X-Mimecast-MFC-AGG-ID":"v9rp1wX1NoalBexJnLn0EQ_1747074885","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1747074885; x=1747679685;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=N3jTJdor9vUGopqt2B019v8z6vmlhEl8FtEuNkD4PmM=;\n\tb=PyL3OhdaumIP9qsLKlhW6BiZkzddfsrSYBn+AD6GxtBoRQ6PMKUnRPt3PqTnXneS0e\n\tMRwgiczSIcaazD8IneCce0pv24PdyVssntKvgMlnTIzf6phxUJCb/VzAeIjPHgA0nsLw\n\tDVNsg7kbiIGSdQXHDKBPF9lxJU5TTU1tAjcpNfYPVyC7mcVgMTZki+1VQLlDejOAExXq\n\tvkKddTgshHneFrrj2PWoOEcvp595oLdDTA0Lpq1vJQeZDiqTCoWZFzJl8eW3nqDC/Bwb\n\tgj3YR4xj5lmRmqKcptMDgGyOfaAag76DtqTKL9e3MfywlbYSlHzNyuOGSXSdvCvIo0VH\n\tyiqg==","X-Gm-Message-State":"AOJu0YwqBf3xb2O2WULLeRqgbbuA0GRA1s9IWgI1Tb7kjqrzhLB6pJxe\n\tR8/0iaPGb6cn/kg+QKEbgXSrreHh1pi4CJDDREjc4gn+lwKzfTwK4gSuB3OUBRukEFHfurqedYw\n\t7WFdrMAxN/xtSLtdpdpDkUVQ4KZIJzz3t9rtuO1cSbWC3TTClFqbQzWPDeaCHp/D7mu9NSnY=","X-Gm-Gg":"ASbGncsQygkZYCpJfwTL6yoxo9FHy0OqNUqEB5bYJl9lKpTpFdjxB5T2CMfF1/HCtOO\n\ts2O5r9mPCiInXfMRJOl/5sgejMBJvnlw2PqUMY504shb+mKdKjIVGf7BLovCt8ZVnc4ZNPqsaT0\n\tZjZEPRjjqp5lBszI+3JlTGgLOzJD9W/IWL5BdbMe68x8qKyVVML0qTOm+9jBUwv5zrIkEMewejh\n\tGHf1a4MdKWF3AXfcmF7j3ogYAgWtjenqGEZXn2Tv+Rn8FAKke+SeU2pRPVFwLmbi9MjWnghUBG7\n\taud3O7gXhWFecWJtiBKl7qnz1k671bFgFuMTh47HRoqLE0pB16meUdVW+Q2733tP","X-Received":["by 2002:a17:907:7d91:b0:ad2:3bfc:236 with SMTP id\n\ta640c23a62f3a-ad23bfc9fe0mr911076266b.22.1747074885211; \n\tMon, 12 May 2025 11:34:45 -0700 (PDT)","by 2002:a17:907:7d91:b0:ad2:3bfc:236 with SMTP id\n\ta640c23a62f3a-ad23bfc9fe0mr911073466b.22.1747074884650; \n\tMon, 12 May 2025 11:34:44 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IG7U+K3dajfnzmqKrYRCkg4BBaaoRhYJ1ZtUIOvoY2rWgdp0C2VB0PIdSnDEmxeXiQQuAhKOQ==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Laurent Pinchart\n\t","Subject":"Re: [PATCH v8 00/15] Add global configuration file","In-Reply-To":" (\n\t=?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Thu,\n\t1 May 2025 13:15:39 +0200\")","References":"<20250430121449.39910-1-mzamazal@redhat.com>\n\t","Date":"Mon, 12 May 2025 20:34:43 +0200","Message-ID":"<858qn1ptj0.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":"sKq4tRX0eCLMHTIozWZwUbNfyULQL1AsfABf1KLi3dY_1747074885","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":34371,"web_url":"https://patchwork.libcamera.org/comment/34371/","msgid":"<85ldqgdcsu.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-05-28T14:34:25","subject":"Re: [PATCH v8 00/15] Add global configuration file","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Barnabás,\n\nMilan Zamazal writes:\n\n> Hi Barnabás,\n>\n> Barnabás Pőcze writes:\n>\n>> Hi\n>>\n>>\n>> 2025. 04. 30. 14:14 keltezéssel, Milan Zamazal írta:\n>>> This patch series introduces global configuration file for libcamera, to\n>>> provide runtime configuration means other than environment variables.\n>>> Instead of, or in addition to, the growing list of configuration\n>>> environment variables, the whole configuration can be specified in a\n>>> single configuration file. This is both simpler and more flexible.\n>>> This is not a replacement for specific configuration files already\n>>> present in libcamera.\n>>> The patches implement what is needed to introduce a configuration file\n>>> that can handle the current environment variables and software ISP\n>>> TODOs. They demonstrate how to deal with the key points that must be\n>>> considered. See commit messages for more details.\n>>> The configuration file is a YAML file. It is looked up in user’s home\n>>> directory or, if not present, in system-wide libcamera directories.\n>>> Both the directories and file name of the configuration file can be\n>>> overridden using newly introduced environment variables.\n>>> The current configuration environment variables are still supported and\n>>> take precedence, if defined, over the corresponding entries in the YAML\n>>> file.\n>>> This patch series is not exhaustive, there can be future enhancements,\n>>> most notably configuration file validation to avoid confusions caused by\n>>> typos etc.\n>>> Not everything has been tested because some of the patches are related\n>>> to specific hardware.\n>>> Also, not everything is necessarily C++ elegant, suggestions for\n>>> improvements are welcome. Note that there is an obstacle in that\n>>> configuration initialization may call logging and logging queries\n>>> configuration, as explained in the commit messages. This complicates\n>>> transparent encapsulation.\n>>\n>> I think generally the direction is to eliminate singletons from libcamera.[0][1]\n>> So I am not sure that a new one should be introduced. I believe we should proceed\n>> towards a design where the configuration is per-CameraManager (e.g. the constructor\n>> loads the configuration), and then it is propagated to other components. (As far as\n>> I can see, this shouldn't be too much trouble: pipeline handlers can reach the CameraManager\n>> easily, and `DebayerCpu` can receive the settings in its constructor via `SoftwareISP`\n>> from the simple pipeline handler, `IPAManager` can receive it in its constructor).\n>\n> I'm not sure we want the configuration being bound to CameraManager; the\n> configuration can contain anything and logging may be just one example\n> of something more global. I'd prefer to avoid reworking the\n> configuration mechanism in future.\n>\n>> Secondly, I would consider dropping the logging related settings from the configuration\n>> file, at least for now. It is likely not the most interesting part, \n>\n> I think logging belongs to the configuration and is a good thing to\n> think about as a proof of the design. Even if we possibly eventually\n> decide to omit it due to technical difficulties, we should think about\n> the overall design more first and have a good answer to the question\n> \"why other software can configure logging in its configuration file and\n> libcamera not?\".\n>\n>> and even with the singleton design, I am not fan of the quite fragile\n>> `GlobalConfiguration::initialize()` call in the `IPAManager`\n>> constructor.\n>\n> I suppose nobody is but the question is how to do better without\n> imposing artificial restrictions on the configuration and/or introducing\n> another ugliness.\n>\n>> (With per-CameraManager configuration, I could\n>> imagine that the constructor initializes the logger based on the configuration. Of\n>> course if two CameraManagers load different configuration, then it is entirely possible\n>> that they will overwrite each other's log settings, but unless we want to inject\n>> separate logger objects into each object of interest, I don't really see a nice\n>> solution; but it's probably not a big issue anyways.)\n>\n> From the practical point of view, the problem of different\n> configurations is indeed unlikely to be a big issue, the configuration\n> would change only if the user edited it. But a design raising such\n> questions smells and sounds like a call for trouble (configuration\n> changing in runtime unexpectedly, possibly in incompatible/unexpected\n> ways, ...), I'd rather avoid it.\n>\n>> Thoughts?\n\nWe discussed it at the software ISP sync today and decided to follow\nyour proposal as it looks better than the current implementation and\nthere is no better idea. I'll make v9 storing the configuration in\nCameraManager.\n\n>> [0]: https://bugs.libcamera.org/show_bug.cgi?id=246\n>> [1]: https://lists.libcamera.org/pipermail/libcamera-devel/2025-April/049888.html\n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>> Changes in v8:\n>>> - Rebased on current master.\n>>> - Anniversary edition: 400 days since v1 posted. ;-)\n>>> Changes in v7:\n>>> - Rebased on current master.\n>>> - Tuning file path configuration updated for recent changes.\n>>> A significant change is that the tuning file configuration is no\n>>> longer sensor dependent as there is apparently no access to the sensor\n>>> info in the IPA proxy.\n>>> - Minor improvements of some commit messages.\n>>> Changes in v6:\n>>> - Rebased on master.\n>>> - File names from file header descriptions removed.\n>>> - Indentation fix in moved code as requested by checkstyle.py.\n>>> - Unneeded const_cast's removed.\n>>> - Using GlobalConfiguration namespace rather than a class.\n>>> - Path configuration options are defined as sequences in YAML.\n>>> - A patch introducing LIBCAMERA_CONFIG_NAME added.\n>>> - A patch introducing LIBCAMERA_CONFIG_DIR added.\n>>> - Miscellaneous minor code changes suggested by Barnabás.\n>>> Changes in v5:\n>>> - A pointer is used to store the global configuration singleton rather\n>>> than a static variable. This makes the things more robust and fixes\n>>> the problem with re-entrancy on logging and a failing\n>>> camera_reconfigure test.\n>>> - In relation to the change above, a new initialization method\n>>> GlobalConfiguration::initialize() was introduced that replaces the\n>>> initialization calls in CameraManager and IPAManager.\n>>> - Logging YAML errors when reading the configuration was also fixed.\n>>> - Global configuration is placed to base directly, without an\n>>> intermediate patch.\n>>> - An `optional' value comparison simplified.\n>>> - A temporary typo in a comment fixed.\n>>> - Unused stdint.h include removed.\n>>> Changes in v4:\n>>> - Rebased on current master.\n>>> - Configuration option for LIBCAMERA_IPA_PROXY_PATH added.\n>>> - Added a patch to include stdlib.h instead of cstdlib in yaml_parser.cpp.\n>>> Changes in v3:\n>>> - Added a configuration item for the newly introduced\n>>> LIBCAMERA_PIPELINES_MATCH_LIST variable.\n>>> - A minor indentation fix.\n>>> - Fixed `pipelines.' x `pipeline.' configuration item naming mismatch.\n>>> - Tuning files are looked up now by particular cameras attached rather than\n>>> being specified for the whole pipeline.\n>>> - Helpers use std::string& instead of char* for confPath arguments.\n>>> - Protection against returning YamlObject::empty as a regular value (the\n>>> problem has been probably exposed by addition of\n>>> LIBCAMERA_PIPELINES_MATCH_LIST).\n>>> Changes in v2:\n>>> - Rebased on master.\n>>> - Various cleanups, documentation improvements and minor fixes.\n>>> - Configuration option for LIBCAMERA_RPI_TUNING_FILE added (Naush).\n>>> - Two more patches for software ISP configuration added.\n>>> Milan Zamazal (15):\n>>> yaml: Move yaml_parser.cpp to base\n>>> config: Introduce global runtime configuration\n>>> config: Look up logging levels in the configuration file\n>>> config: Add configuration retrieval helpers\n>>> config: Look up log file in configuration file\n>>> config: Look up log color configuration in configuration file\n>>> config: Look up rpi config path in configuration file\n>>> config: Look up IPA configurables in configuration file\n>>> config: Look up pipelines match list in configuration file\n>>> config: Allow enabling software ISP in runtime\n>>> config: Add global configuration file documentation\n>>> libcamera: software_isp: Make input buffer copying configurable\n>>> libcamera: software_isp: Make measurement configurable\n>>> config: Make configuration file configurable\n>>> config: Make configuration directories configurable\n>>> Documentation/documentation-contents.rst | 2 +-\n>>> Documentation/index.rst | 2 +-\n>>> Documentation/meson.build | 2 +-\n>>> ...ariables.rst => runtime_configuration.rst} | 133 +++++++-\n>>> .../libcamera/internal/global_configuration.h | 60 ++++\n>>> include/libcamera/internal/meson.build | 1 +\n>>> src/libcamera/base/global_configuration.cpp | 289 ++++++++++++++++++\n>>> src/libcamera/base/log.cpp | 40 +--\n>>> src/libcamera/base/meson.build | 15 +\n>>> src/libcamera/{ => base}/yaml_parser.cpp | 13 +-\n>>> src/libcamera/camera_manager.cpp | 10 +-\n>>> src/libcamera/ipa_manager.cpp | 35 ++-\n>>> src/libcamera/ipa_proxy.cpp | 65 ++--\n>>> src/libcamera/meson.build | 14 -\n>>> .../pipeline/rpi/common/pipeline_base.cpp | 8 +-\n>>> src/libcamera/pipeline/simple/simple.cpp | 12 +\n>>> src/libcamera/process.cpp | 11 +-\n>>> src/libcamera/software_isp/TODO | 36 ---\n>>> src/libcamera/software_isp/debayer_cpu.cpp | 30 +-\n>>> src/libcamera/software_isp/debayer_cpu.h | 7 +-\n>>> 20 files changed, 630 insertions(+), 155 deletions(-)\n>>> rename Documentation/{environment_variables.rst => runtime_configuration.rst} (58%)\n>>> create mode 100644 include/libcamera/internal/global_configuration.h\n>>> create mode 100644 src/libcamera/base/global_configuration.cpp\n>>> rename src/libcamera/{ => base}/yaml_parser.cpp (98%)\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 D1384C31E9\n\tfor ;\n\tWed, 28 May 2025 14:34:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ECDC768DA0;\n\tWed, 28 May 2025 16:34:32 +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 F296F614CE\n\tfor ;\n\tWed, 28 May 2025 16:34:31 +0200 (CEST)","from mail-ej1-f70.google.com (mail-ej1-f70.google.com\n\t[209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-54-iM63-g0jMUSuYVPsKb5kMw-1; Wed, 28 May 2025 10:34:29 -0400","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-ad8a0eef39bso61200666b.3\n\tfor ;\n\tWed, 28 May 2025 07:34:28 -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\ta640c23a62f3a-ad8a19ace35sm116620966b.7.2025.05.28.07.34.25\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 28 May 2025 07:34: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=\"YDk+Ss/o\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1748442870;\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=oHdGMRZzH0DrNzeyM1XzAxVr4fQtTQtjQFe4EVEwycU=;\n\tb=YDk+Ss/oepIknN6jgRzwbsDgBcJwoOlxScZo9HuABTH6QKim5hzG78x+CU99nei20/Le6n\n\trL8RVNpMSuC71s7IO2HqLUf6PtQRb3xk/8ZwyOnxomfWZRB76n7vIcREaTPcMIRGdyqe5E\n\txdMMSpwELcVPXRieddYf20pQHpweSno=","X-MC-Unique":"iM63-g0jMUSuYVPsKb5kMw-1","X-Mimecast-MFC-AGG-ID":"iM63-g0jMUSuYVPsKb5kMw_1748442867","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1748442867; x=1749047667;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=UUDRiTfK5krdhVTI5jYaYmCIODOq3K4LajLkYm25lDo=;\n\tb=t0WkJp1NAnQ0G1duw+I+A7BQJdqwULlO2RIH16RRZElQ9sHwsNCu5+YUw1gEFkm0s1\n\tDsu4FpP4yTvU226G0ugaB/jMdgl+Q0+DEqcroWpkyWwHjObxWd7dCJ+5o33iXxMWpbtb\n\tV8v7lx6OaLbd44aa/JlavSdFq4BGA1/Lw0ygijhGJAvaU1N8BbITEzO9ZnzZ45+tOW/r\n\t9izf1/1cOwErnh094StGi5HRkmDAwRBLhHF4dPIidf1qcJ1RcYweR79npcvXLtCEGpl5\n\tYxWn6eWzA3/wWvzc96uvkz/SaB+0lUbGiDGEwlmkg28z+8V2MmKRDlHgxFe64md/Npmj\n\tZiJg==","X-Gm-Message-State":"AOJu0YzYULHQjpKZpUDgO9SgsmQtDjegRnp35ehFHY5n0q4RVGszwRTW\n\tFQHPDMM1OHNnS3Sm/y+fmINk1mFEKxFVGrGyy8Faj0uuaSd+GN1LvwIP1vB/wxR8eCC2U5yIAf7\n\twec1gLBVwk8zESmosUqsSFqKcyryF/d6QNsh6P+rzPObnJc7Rcj4btcGNQKq/Wm0KzbvoHepe84\n\tY=","X-Gm-Gg":"ASbGncsYytYrKgy3kmWAnUVpHXpPIRlIQJNuk39eSKOmelOX9rrWK4cSVsM2BgtzlB/\n\t87SxkiwFa3JdLU0WV6wZCHh+2NSIIkasJCqmFdK/aNqtSWBHtqQNLzwC9UXIARohQjcnyrrsQOZ\n\tDmmRIJv5n1AJCCxCwu6lv7ceKJRle9M2e5tTPfzhJ9yQm8L2ORw/vrLmZd2I+sDpAVLiUXB9xeT\n\txxnGpsLalBSRJIH74HzZjPjk9tXMLFDohYcf2+zknUQtBvotR/neO3dr0wx7bXJHDnGWM3b24kX\n\tTiGq+l/t3a5SWrm9JDPayhildbJX+jsKALWIVm5Tl5kfNYtSUt9JAS5mp8oAcrKT","X-Received":["by 2002:a17:907:9802:b0:ac3:3e40:e183 with SMTP id\n\ta640c23a62f3a-ad85b12fd26mr1500544466b.3.1748442866886; \n\tWed, 28 May 2025 07:34:26 -0700 (PDT)","by 2002:a17:907:9802:b0:ac3:3e40:e183 with SMTP id\n\ta640c23a62f3a-ad85b12fd26mr1500542266b.3.1748442866428; \n\tWed, 28 May 2025 07:34:26 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFJqXlRSwkk2TTm1zBOSZYtB590kfa1q/iLGkCoJ6EZHzxzEMNZmNgT73AgDnAl1YFhaInMlA==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Laurent Pinchart\n\t","Subject":"Re: [PATCH v8 00/15] Add global configuration file","In-Reply-To":"<858qn1ptj0.fsf@mzamazal-thinkpadp1gen7.tpbc.csb> (Milan\n\tZamazal's message of \"Mon, 12 May 2025 20:34:43 +0200\")","References":"<20250430121449.39910-1-mzamazal@redhat.com>\n\t\n\t<858qn1ptj0.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Date":"Wed, 28 May 2025 16:34:25 +0200","Message-ID":"<85ldqgdcsu.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":"Wz_wKni3SNZy4vOG-C9-8YIynLgdJDkNNGAv3ybLaIM_1748442867","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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\" "}}]