| Message ID | 20260113000808.15395-37-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
2026. 01. 13. 1:08 keltezéssel, Laurent Pinchart írta: > The pipelines.simple.devices configuration option contains a list of > devices, each of them being a dictionary with a "driver" element that > acts as a unique key to index the list. Turn it into a YAML dictionary > to ensure uniqueness of the key, and simplify access in the pipeline > handler. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > This simplification will preclude indexing the list of devices with a > compound key. For instance, if we were to extend the simple pipeline > handler to match not only on a driver name but on a (driver, driver > version) pair, the current configuration file structure would allow > expressing this with > > pipelines: > simple: > devices: > - driver: mxc-isi > driver_version: 1 > software_isp: true > - driver: mxc-isi > driver_version: 2 > software_isp: false > > The new structure would conceptually allow us to write > > pipelines: > simple: > devices: > ? driver: mxc-isi > driver_version: 1 > : software_isp: true > ? driver: mxc-isi > driver_version: 2 > : software_isp: false > > For those not familiar with the explicit syntax for mappings, this would > translate to the following Python data if dict objects were hashable in > Python: > > { > 'pipelines': { > 'simple': { > 'devices': { > { 'driver': 'mxc-isi', 'driver_version': 1 }: { > 'software_isp': True, > }, > { 'driver': 'mxc-isi', 'driver_version': 2 }: { > 'software_isp': False, > }, > }, > }, > }, > } > > Yes, YAML can use complex data as keys in mappings. I would really not > want to have to support that in libcamera though. Generally I feel like adopting a different approach would be more flexible. Specifically, the the user could define a list of rules that have associated conditions and actions. For example: - if: driver: mxc-isi driver_version: 1 then: software_isp: true - if: driver: mxc-isi driver_version: '>=2' then: software_isp: false then these rules are evaluated in order of appearance to get the final list of properties/options for the given device. I believe this is easier to handle in a generic way than compound keys and it allows one to support e.g. regex matching, number ranges, etc. should the need arise. > > Opinions will be appreciated. > --- > Documentation/runtime_configuration.rst | 6 +++--- > src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++----------- > 2 files changed, 12 insertions(+), 14 deletions(-) > > diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst > index 19c2309ac94f..a904b09a1d35 100644 > --- a/Documentation/runtime_configuration.rst > +++ b/Documentation/runtime_configuration.rst > @@ -44,7 +44,7 @@ file structure: > pipelines: > simple: > devices: > - - driver: # driver name, e.g. `mxc-isi` > + # driver name, e.g. `mxc-isi`: > software_isp: # true/false > software_isp: > copy_input_buffer: # true/false > @@ -77,7 +77,7 @@ Configuration file example > pipelines: > simple: > devices: > - - driver: mxc-isi > + mxc-isi: > software_isp: true > software_isp: > copy_input_buffer: false > @@ -139,7 +139,7 @@ LIBCAMERA_<NAME>_TUNING_FILE > > Example value: ``/usr/local/share/libcamera/ipa/rpi/vc4/custom_sensor.json`` > > -pipelines.simple.devices.driver, pipelines.simple.devices.software_isp > +pipelines.simple.devices.<driver>.software_isp > Override whether software ISP is enabled for the given driver. > > Example `driver` value: ``mxc-isi`` > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 0ae9e081f01a..c949f3a69d16 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -1879,18 +1879,16 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media, > } > > swIspEnabled_ = info.swIspEnabled; > + > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); > - for (GlobalConfiguration::Option entry : > - configuration.configuration()["pipelines"]["simple"]["devices"] > - .asList()) { > - auto name = entry["driver"].get<std::string>(); > - if (name == info.driver) { > - swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_); > - LOG(SimplePipeline, Debug) > - << "Configuration file overrides software ISP for " > - << info.driver << " to " << swIspEnabled_; > - break; > - } > + GlobalConfiguration::Option &cfg = > + configuration.configuration()["pipelines"]["simple"]["devices"][info.driver]; > + > + if (cfg) { > + swIspEnabled_ = cfg["software_isp"].get<bool>().value_or(swIspEnabled_); > + LOG(SimplePipeline, Debug) > + << "Configuration file overrides software ISP for " > + << info.driver << " to " << swIspEnabled_; Maybe if (auto x = cfg["software_isp"].get<bool>()) could be used in the condition. Since the current and proposed versions both print the message about the override even if no override actually happens (due to missing/malformed `sofware_isp` key). Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > } > > /* Locate the sensors. */
On Wed, Jan 14, 2026 at 10:33:44AM +0100, Barnabás Pőcze wrote: > 2026. 01. 13. 1:08 keltezéssel, Laurent Pinchart írta: > > The pipelines.simple.devices configuration option contains a list of > > devices, each of them being a dictionary with a "driver" element that > > acts as a unique key to index the list. Turn it into a YAML dictionary > > to ensure uniqueness of the key, and simplify access in the pipeline > > handler. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > This simplification will preclude indexing the list of devices with a > > compound key. For instance, if we were to extend the simple pipeline > > handler to match not only on a driver name but on a (driver, driver > > version) pair, the current configuration file structure would allow > > expressing this with > > > > pipelines: > > simple: > > devices: > > - driver: mxc-isi > > driver_version: 1 > > software_isp: true > > - driver: mxc-isi > > driver_version: 2 > > software_isp: false > > > > The new structure would conceptually allow us to write > > > > pipelines: > > simple: > > devices: > > ? driver: mxc-isi > > driver_version: 1 > > : software_isp: true > > ? driver: mxc-isi > > driver_version: 2 > > : software_isp: false > > > > For those not familiar with the explicit syntax for mappings, this would > > translate to the following Python data if dict objects were hashable in > > Python: > > > > { > > 'pipelines': { > > 'simple': { > > 'devices': { > > { 'driver': 'mxc-isi', 'driver_version': 1 }: { > > 'software_isp': True, > > }, > > { 'driver': 'mxc-isi', 'driver_version': 2 }: { > > 'software_isp': False, > > }, > > }, > > }, > > }, > > } > > > > Yes, YAML can use complex data as keys in mappings. I would really not > > want to have to support that in libcamera though. > > Generally I feel like adopting a different approach would be more flexible. > Specifically, the the user could define a list of rules that have associated > conditions and actions. For example: > > - if: > driver: mxc-isi > driver_version: 1 > then: > software_isp: true > - if: > driver: mxc-isi > driver_version: '>=2' > then: > software_isp: false > > then these rules are evaluated in order of appearance to get the final list of properties/options > for the given device. I believe this is easier to handle in a generic way than compound keys and > it allows one to support e.g. regex matching, number ranges, etc. should the need arise. Compound keys is definitely not something I would want to implement, the complexity is just not worth it. Rules would be easier, but they would still require quite a bit of implementation effort. Given your R-b tag below I assume you're not recommending to implement this right now. > > > > Opinions will be appreciated. > > --- > > Documentation/runtime_configuration.rst | 6 +++--- > > src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++----------- > > 2 files changed, 12 insertions(+), 14 deletions(-) > > > > diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst > > index 19c2309ac94f..a904b09a1d35 100644 > > --- a/Documentation/runtime_configuration.rst > > +++ b/Documentation/runtime_configuration.rst > > @@ -44,7 +44,7 @@ file structure: > > pipelines: > > simple: > > devices: > > - - driver: # driver name, e.g. `mxc-isi` > > + # driver name, e.g. `mxc-isi`: > > software_isp: # true/false > > software_isp: > > copy_input_buffer: # true/false > > @@ -77,7 +77,7 @@ Configuration file example > > pipelines: > > simple: > > devices: > > - - driver: mxc-isi > > + mxc-isi: > > software_isp: true > > software_isp: > > copy_input_buffer: false > > @@ -139,7 +139,7 @@ LIBCAMERA_<NAME>_TUNING_FILE > > > > Example value: ``/usr/local/share/libcamera/ipa/rpi/vc4/custom_sensor.json`` > > > > -pipelines.simple.devices.driver, pipelines.simple.devices.software_isp > > +pipelines.simple.devices.<driver>.software_isp > > Override whether software ISP is enabled for the given driver. > > > > Example `driver` value: ``mxc-isi`` > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 0ae9e081f01a..c949f3a69d16 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -1879,18 +1879,16 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media, > > } > > > > swIspEnabled_ = info.swIspEnabled; > > + > > const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); > > - for (GlobalConfiguration::Option entry : > > - configuration.configuration()["pipelines"]["simple"]["devices"] > > - .asList()) { > > - auto name = entry["driver"].get<std::string>(); > > - if (name == info.driver) { > > - swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_); > > - LOG(SimplePipeline, Debug) > > - << "Configuration file overrides software ISP for " > > - << info.driver << " to " << swIspEnabled_; > > - break; > > - } > > + GlobalConfiguration::Option &cfg = > > + configuration.configuration()["pipelines"]["simple"]["devices"][info.driver]; > > + > > + if (cfg) { > > + swIspEnabled_ = cfg["software_isp"].get<bool>().value_or(swIspEnabled_); > > + LOG(SimplePipeline, Debug) > > + << "Configuration file overrides software ISP for " > > + << info.driver << " to " << swIspEnabled_; > > Maybe > > if (auto x = cfg["software_isp"].get<bool>()) > > could be used in the condition. Since the current and proposed versions both print the message > about the override even if no override actually happens (due to missing/malformed `sofware_isp` key). Good point. I'll fix it. > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > } > > > > /* Locate the sensors. */
diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst index 19c2309ac94f..a904b09a1d35 100644 --- a/Documentation/runtime_configuration.rst +++ b/Documentation/runtime_configuration.rst @@ -44,7 +44,7 @@ file structure: pipelines: simple: devices: - - driver: # driver name, e.g. `mxc-isi` + # driver name, e.g. `mxc-isi`: software_isp: # true/false software_isp: copy_input_buffer: # true/false @@ -77,7 +77,7 @@ Configuration file example pipelines: simple: devices: - - driver: mxc-isi + mxc-isi: software_isp: true software_isp: copy_input_buffer: false @@ -139,7 +139,7 @@ LIBCAMERA_<NAME>_TUNING_FILE Example value: ``/usr/local/share/libcamera/ipa/rpi/vc4/custom_sensor.json`` -pipelines.simple.devices.driver, pipelines.simple.devices.software_isp +pipelines.simple.devices.<driver>.software_isp Override whether software ISP is enabled for the given driver. Example `driver` value: ``mxc-isi`` diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 0ae9e081f01a..c949f3a69d16 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -1879,18 +1879,16 @@ bool SimplePipelineHandler::matchDevice(std::shared_ptr<MediaDevice> media, } swIspEnabled_ = info.swIspEnabled; + const GlobalConfiguration &configuration = cameraManager()->_d()->configuration(); - for (GlobalConfiguration::Option entry : - configuration.configuration()["pipelines"]["simple"]["devices"] - .asList()) { - auto name = entry["driver"].get<std::string>(); - if (name == info.driver) { - swIspEnabled_ = entry["software_isp"].get<bool>().value_or(swIspEnabled_); - LOG(SimplePipeline, Debug) - << "Configuration file overrides software ISP for " - << info.driver << " to " << swIspEnabled_; - break; - } + GlobalConfiguration::Option &cfg = + configuration.configuration()["pipelines"]["simple"]["devices"][info.driver]; + + if (cfg) { + swIspEnabled_ = cfg["software_isp"].get<bool>().value_or(swIspEnabled_); + LOG(SimplePipeline, Debug) + << "Configuration file overrides software ISP for " + << info.driver << " to " << swIspEnabled_; } /* Locate the sensors. */
The pipelines.simple.devices configuration option contains a list of devices, each of them being a dictionary with a "driver" element that acts as a unique key to index the list. Turn it into a YAML dictionary to ensure uniqueness of the key, and simplify access in the pipeline handler. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- This simplification will preclude indexing the list of devices with a compound key. For instance, if we were to extend the simple pipeline handler to match not only on a driver name but on a (driver, driver version) pair, the current configuration file structure would allow expressing this with pipelines: simple: devices: - driver: mxc-isi driver_version: 1 software_isp: true - driver: mxc-isi driver_version: 2 software_isp: false The new structure would conceptually allow us to write pipelines: simple: devices: ? driver: mxc-isi driver_version: 1 : software_isp: true ? driver: mxc-isi driver_version: 2 : software_isp: false For those not familiar with the explicit syntax for mappings, this would translate to the following Python data if dict objects were hashable in Python: { 'pipelines': { 'simple': { 'devices': { { 'driver': 'mxc-isi', 'driver_version': 1 }: { 'software_isp': True, }, { 'driver': 'mxc-isi', 'driver_version': 2 }: { 'software_isp': False, }, }, }, }, } Yes, YAML can use complex data as keys in mappings. I would really not want to have to support that in libcamera though. Opinions will be appreciated. --- Documentation/runtime_configuration.rst | 6 +++--- src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++----------- 2 files changed, 12 insertions(+), 14 deletions(-)