[libcamera-devel] [PATCH v3 03/23] libcamera: Introduce internal controls

[Jacopo Mondi]

Hi Kieran

On Thu, Jun 30, 2022 at 10:06:27PM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:42)
> > Introduce the enumeration of internal controls in
> > internal_control_ids.yaml.
> >
> > Plumb in the build system the command to generate the definition of
> > internal controls by re-using the same mechanism used for public
> > controls to make it easy to extend it to also handle internal properties
> > in future.
>
> I'm afraid these are bugging me ... Perhaps I missed this before when we
> discussed internal controls...
>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at yoseli.org>
> > ---
> >  include/libcamera/internal/meson.build  | 15 ++++++++++++++
> >  src/libcamera/internal_control_ids.yaml | 27 +++++++++++++++++++++++++
> >  src/libcamera/meson.build               | 17 ++++++++++++++++
> >  3 files changed, 59 insertions(+)
> >  create mode 100644 src/libcamera/internal_control_ids.yaml
> >
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 7a780d48ee57..1559c3c368c4 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -9,6 +9,21 @@ libcamera_tracepoint_header = custom_target(
> >      command: [gen_tracepoints_header, '@OUTPUT@', '@INPUT@'],
> >  )
> >
> > +# Generate the list of internal controls identifiers
> > +internal_control_source_files = ['control_ids']
> > +
> > +libcamera_internal_control_headers = []
> > +
> > +foreach header : internal_control_source_files
> > +    input_files = files('../../../src/libcamera/internal_' + header +'.yaml',\
> > +                        '../' + header + '.h.in')
> > +    libcamera_internal_control_headers += custom_target(
> > +            'internal_' + header + '_h',
> > +            input : input_files,
> > +            output : header + '.h',
> > +            command : [gen_controls, '--internal=True','-o', '@OUTPUT@', '@INPUT@'])
> > +endforeach
> > +
> >  libcamera_internal_headers = files([
> >      'bayer_format.h',
> >      'byte_stream_buffer.h',
> > diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml
> > new file mode 100644
> > index 000000000000..6d3775afcf67
> > --- /dev/null
> > +++ b/src/libcamera/internal_control_ids.yaml
> > @@ -0,0 +1,27 @@
> > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > +#
> > +# Copyright (C) 2022, Google Inc.
> > +#
> > +%YAML 1.2
> > +---
> > +# Enumeration of internal libcamera controls
> > +# Not exposed to application, for library use only
> > +
> > +controls:
> > +
> > +  - ExposureTime:
>
> We have: libcamera::controls::ExposureTime already
> https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#a4e1ca45653b62cd969d4d67a741076eb
>
> > +      type: int32_t
> > +      description: |
> > +        The sensor exposure time in milliseconds.
> > +
> > +  - FrameDuration:
>
>
> And libcamera::controls::FrameDuration
>
> https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#a37d99a76c7249c143beecd70917469be
>
>
>
> > +      type: int64_t
> > +      description: |
> > +        The sensor frame duration time in milliseconds.
> > +
> > +  - AnalogueGain:
>
> And ... libcamera::controls::AnalogueGain
>
> https://libcamera.org/api-html/namespacelibcamera_1_1controls.html#ab34ebeaa9cbfb3f3fc6996b089ca52b0
>
> These are 'metadata' controls that are supposed to be returned by the
> Pipeline handler in the metadata. But they mean exactly the same thing
> here.
>
> So, is there a reason we aren't using them directly?

Yes, the names are the same.

Although the application visibile controls have additional semantic
that the internal/camera_sensor controls do not have.

controls::ExposureTime in example is not metadata only, but (will,
once we finalize the AE controls rework) actually drive how the
IPA handles auto/manual exposure, something which does not apply to
the internal version.

So we should append to each of the application visible controls a
paragraph about their usage to drive the camera sensor, and we already
have no clear division between controls/metadata in their definition
which is already confusing sometimes.

So, to me it's seems more appropriate to separate the controls space,
also to let the two interface evolve separately, but maybe I'm just
over-concerned and we could just re-use the public controls without
even mentioning they're used internally, as applications do not care
after all.

>
> --
> Kieran
>
> > +      type: float
> > +      description: |
> > +        The sensor analogue gain value.
> > +
> > +...
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index b57bee7ef6ca..89fdf347c708 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -52,6 +52,7 @@ libcamera_sources = files([
> >  libcamera_sources += libcamera_public_headers
> >  libcamera_sources += libcamera_generated_ipa_headers
> >  libcamera_sources += libcamera_tracepoint_header
> > +libcamera_sources += libcamera_internal_control_headers
> >
> >  includes = [
> >      libcamera_includes,
> > @@ -99,6 +100,7 @@ if not libyaml.found()
> >      libyaml = libyaml_wrap.dependency('yaml')
> >  endif
> >
> > +# Generate control_ids.cpp and property_ids.cpp
> >  control_sources = []
> >
> >  foreach source : control_source_files
> > @@ -111,6 +113,21 @@ endforeach
> >
> >  libcamera_sources += control_sources
> >
> > +# Generate internal_control_ids.cpp
> > +internal_control_source_files = ['control_ids']
> > +internal_control_sources = []
> > +
> > +foreach source : internal_control_source_files
> > +    input_files = files('internal_' + source +'.yaml', source + '.cpp.in')
> > +    internal_control_sources += custom_target('internal_' + source + '_cpp',
> > +                                              input : input_files,
> > +                                              output : 'internal_' + source + '.cpp',
> > +                                              command : [gen_controls, '--internal=True',\
> > +                                                                       '-o', '@OUTPUT@',\
> > +                                                                       '@INPUT@'])
> > +endforeach
> > +libcamera_sources += internal_control_sources
> > +
> >  gen_version = meson.project_source_root() / 'utils' / 'gen-version.sh'
> >
> >  # Use vcs_tag() and not configure_file() or run_command(), to ensure that the
> > --
> > 2.36.1
> >