[libcamera-devel] [PATCH 1/9] libcamera: Introduce option to customize behavior for a camera module

[Laurent Pinchart]

Hi Han-lin,

On Wed, Feb 09, 2022 at 03:19:09PM +0800, Han-Lin Chen wrote:
> Introduce option to customize behavior for a camera module, which might
> alter the camera characteristic and should be set before inspecting it.
> 
> Re-use the Control generation infrastructure to generate options and define
> the first 'PipelineConfigFile' option, which allows user to specify
> preference for pipeline configuration.

When thinking about this, I wasn't envisioning passing the configuration
file to libcamera through the C++ API, but looking for it in standard
locations on the file system. Could you explain the reason why you think
this should be dynamic ?

> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> ---
>  include/libcamera/ipa/ipa_controls.h |  1 +
>  include/libcamera/meson.build        |  3 +-
>  include/libcamera/option_ids.h.in    | 36 +++++++++++++++++
>  src/libcamera/control_serializer.cpp | 12 ++++++
>  src/libcamera/option_ids.cpp.in      | 58 ++++++++++++++++++++++++++++
>  src/libcamera/option_ids.yaml        | 16 ++++++++
>  6 files changed, 125 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/option_ids.h.in
>  create mode 100644 src/libcamera/option_ids.cpp.in
>  create mode 100644 src/libcamera/option_ids.yaml
> 
> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h
> index da1a7596..8173a2e5 100644
> --- a/include/libcamera/ipa/ipa_controls.h
> +++ b/include/libcamera/ipa/ipa_controls.h
> @@ -18,6 +18,7 @@ extern "C" {
>  enum ipa_controls_id_map_type {
>  	IPA_CONTROL_ID_MAP_CONTROLS,
>  	IPA_CONTROL_ID_MAP_PROPERTIES,
> +	IPA_CONTROL_ID_MAP_OPTIONS,

Do you have use cases for passing a configuration file to the IPA
modules ? As IPA modules are sandboxed, they don't have unrestricted
access to the filesystem. I think this could create issues, especially
if the path to the configuration is under the control of an application.

>  	IPA_CONTROL_ID_MAP_V4L2,
>  };
>  
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 408b7acf..a9b71bc3 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -31,10 +31,11 @@ install_headers(libcamera_public_headers,
>  
>  libcamera_headers_install_dir = get_option('includedir') / libcamera_include_dir
>  
> -# control_ids.h and property_ids.h
> +# control_ids.h, property_ids.h, and option_ids.h
>  control_source_files = [
>      'control_ids',
>      'property_ids',
> +    'option_ids',
>  ]
>  
>  control_headers = []
> diff --git a/include/libcamera/option_ids.h.in b/include/libcamera/option_ids.h.in
> new file mode 100644
> index 00000000..53058a4b
> --- /dev/null
> +++ b/include/libcamera/option_ids.h.in
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * option_ids.h - Option ID list
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#pragma once
> +
> +#include <stdint.h>
> +
> +#include <libcamera/controls.h>
> +
> +namespace libcamera {
> +
> +namespace options {
> +
> +enum {
> +${ids}
> +};
> +
> +${controls}
> +
> +namespace draft {
> +
> +${draft_controls}
> +
> +} /* namespace draft */
> +
> +extern const ControlIdMap options;
> +
> +} /* namespace options */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index e87d2362..dc8d2906 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -16,6 +16,7 @@
>  
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
> +#include <libcamera/option_ids.h>
>  #include <libcamera/property_ids.h>
>  
>  #include <libcamera/ipa/ipa_controls.h>
> @@ -239,6 +240,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
>  	enum ipa_controls_id_map_type idMapType;
>  	if (idmap == &controls::controls)
>  		idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> +	else if (idmap == &options::options)
> +		idMapType = IPA_CONTROL_ID_MAP_OPTIONS;
>  	else if (idmap == &properties::properties)
>  		idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
>  	else
> @@ -333,6 +336,8 @@ int ControlSerializer::serialize(const ControlList &list,
>  	enum ipa_controls_id_map_type idMapType;
>  	if (idmap == &controls::controls)
>  		idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> +	else if (idmap == &options::options)
> +		idMapType = IPA_CONTROL_ID_MAP_OPTIONS;
>  	else if (idmap == &properties::properties)
>  		idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
>  	else
> @@ -458,6 +463,9 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
>  	case IPA_CONTROL_ID_MAP_CONTROLS:
>  		idMap = &controls::controls;
>  		break;
> +	case IPA_CONTROL_ID_MAP_OPTIONS:
> +		idMap = &options::options;
> +		break;
>  	case IPA_CONTROL_ID_MAP_PROPERTIES:
>  		idMap = &properties::properties;
>  		break;
> @@ -589,6 +597,10 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
>  			idMap = &controls::controls;
>  			break;
>  
> +		case IPA_CONTROL_ID_MAP_OPTIONS:
> +			idMap = &options::options;
> +			break;
> +
>  		case IPA_CONTROL_ID_MAP_PROPERTIES:
>  			idMap = &properties::properties;
>  			break;
> diff --git a/src/libcamera/option_ids.cpp.in b/src/libcamera/option_ids.cpp.in
> new file mode 100644
> index 00000000..adfd0633
> --- /dev/null
> +++ b/src/libcamera/option_ids.cpp.in
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * option_ids.cpp : Option ID list
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#include <libcamera/option_ids.h>
> +
> +/**
> + * \file option_ids.h
> + * \brief Camera option identifiers
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \brief Namespace for libcamera options
> + */
> +namespace options {
> +
> +${controls_doc}
> +
> +/**
> + * \brief Namespace for libcamera draft options
> + */
> +namespace draft {
> +
> +${draft_controls_doc}
> +
> +} /* namespace draft */
> +
> +#ifndef __DOXYGEN__
> +/*
> + * Keep the options definitions hidden from doxygen as it incorrectly parses
> + * them as functions.
> + */
> +${controls_def}
> +
> +namespace draft {
> +
> +${draft_controls_def}
> +
> +} /* namespace draft */
> +#endif
> +
> +/**
> + * \brief List of all supported libcamera options
> + */
> +extern const ControlIdMap options {
> +${controls_map}
> +};
> +
> +} /* namespace options */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/option_ids.yaml b/src/libcamera/option_ids.yaml
> new file mode 100644
> index 00000000..3f9304be
> --- /dev/null
> +++ b/src/libcamera/option_ids.yaml
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +#
> +# Copyright (C) 2022, Google Inc.
> +#
> +%YAML 1.2
> +---
> +controls:
> +  - PipelineConfigFile:
> +      type: string
> +      description: |
> +        Some platform supports customizing pipeline configuration preference
> +        for a camera module. The structure and contents of the configuration
> +        file is specific to each platform in YAML format. The PipelineConfigFile
> +        option can be used to specify the path to load the configuration file.

I'm not totally thrilled about using ControlList for this, I feel it may
be a bit over-engineered, but I suppose I'll understand the rationale
better after reviewing the whole series.

> +
> +...

-- 
Regards,

Laurent Pinchart