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

[Kieran Bingham]

Quoting Jacopo Mondi (2022-07-01 09:25:13)
> 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.

Well - to me the distinction would be ... could the metadata and the
internal control ever have a different value to mean the same thing.
That would warrant a different control namespace/scope - but as I
understand it here, these internal controls should be the specific
values that are exposed via metadata for each frame.

--
Kieran


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