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

16461 diff mbox series

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.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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

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@jmondi.org>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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?

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

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@jmondi.org>
> > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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
> >

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@jmondi.org>
> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@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
> > >