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

[Hanlin Chen]

Hi Laurent,
Thanks for the comments.

On Wed, Feb 16, 2022 at 8:57 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?

What I had in mind is to let the application choose the preference per
camera module, and even with the same sensor,
the user might still want different tunings. And we don't need to have
a big configuration file for all parameters.
The option may extend to what we used to set in environment variables,
like LIBCAMERA_RPI_TUNING_FILE for example.

Yes, I think it's up for discussion, and depends on how we think it's
useful and whether it can extend.
If this is the only use case, I do agree it's over-engineered :|

>
> > 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.

I've seen cases that read the content of a file and pass it to IPA
with a FD to avoid the problem.
In fact, for ChromeOS, we control the access of IPA to a white-listed
locations, so it's something I can configure.
For other platforms, I think we need a definition of "sandboxed" and
who should do the sandboxing, like libcamera or the device vendor?

>
> >       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