[libcamera-devel] [PATCH 3/3] libcamera: Introduce internal controls

[Jean-Michel Hautbois]

Hi Jacopo,

Thanks for the patch !

On 21/06/2022 17:03, Jacopo Mondi via libcamera-devel wrote:
> Introduce the enumeration of internal controls in
> internal_control_ids.yaml.
> 
> The list of controls currently defines 4 draft controls which mirror the
> definition of the V4L2 control they map to.
> 
> 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 at jmondi.org>
> ---
>   include/libcamera/internal/meson.build  | 18 +++++++++
>   src/libcamera/internal_control_ids.yaml | 54 +++++++++++++++++++++++++
>   src/libcamera/meson.build               | 16 ++++++++
>   3 files changed, 88 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..569940b0d2e8 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -44,3 +44,21 @@ libcamera_internal_headers = files([
>       'v4l2_videodevice.h',
>       'yaml_parser.h',
>   ])
> +
> +# Generate the list of internal controls identifiers
> +internal_control_source_files = ['control_ids']
> +
> +internal_control_headers = []
> +
> +foreach header : internal_control_source_files
> +    input_files = files('../../../src/libcamera/internal_' + header +'.yaml',\
> +                        '../' + header + '.h.in')
> +    internal_control_headers += custom_target('internal_' + header + '_h',
> +                                              input : input_files,
> +                                              output : header + '.h',
> +                                              command : [gen_controls, '--internal=True',\
> +                                                                       '-o', '@OUTPUT@',\
> +                                                                       '@INPUT@'],
> +                                              install : false)
> +endforeach
> +libcamera_internal_headers += internal_control_headers
> diff --git a/src/libcamera/internal_control_ids.yaml b/src/libcamera/internal_control_ids.yaml
> new file mode 100644
> index 000000000000..e69e0d30657c
> --- /dev/null
> +++ b/src/libcamera/internal_control_ids.yaml
> @@ -0,0 +1,54 @@
> +# 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:
> +
> +  # ----------------------------------------------------------------------------
> +  # Draft controls section
> +
> +  - VBlank:
> +      type: int32_t
> +      draft: true
> +      description: |
> +        Vertical blanking. The idle period after every frame during which no
> +        image data is produced. The unit of vertical blanking is a line. Every
> +        line has length of the image width plus horizontal blanking at the pixel
> +        rate defined by V4L2_CID_PIXEL_RATE control in the same sub-device.
> +
> +        Currently identical to V4L2_CID_VBLANK.
> +
> +  - HBlank:
> +      type: int32_t
> +      draft: true
> +      description: |
> +        Horizontal blanking. The idle period after every line of image data
> +        during which no image data is produced. The unit of horizontal blanking
> +        is pixels.
> +
> +        Currently identical to V4L2_CID_HBLANK.
> +
> +  - SensorAnalogueGain:
> +      type: int32_t
> +      draft: true
> +      description: |
> +        Analogue gain is gain affecting all colour components in the pixel
> +        matrix. The gain operation is performed in the analogue domain before
> +        A/D conversion
> +
> +        Currently identical to V4L2_CID_ANALOGUE_GAIN.
> +
> +  - SensorExposure:
> +      type: int32_t
> +      draft: true
> +      description: |
> +        Exposure time, expressed in frame lines.
> +
> +        Currently identical to V4L2_CID_EXPOSURE.

I have a general comment here, could probably be on top of the series. I 
think we should convey the SensorAnalogueGain as a double value and let 
the CameraSensor call the helper to convert it in a gain code. Thus, the 
control would be Gain, and the CameraSensor class (or a V4L2 specific 
version of it ;-)) would then convert this gain into Analogue Gain + 
digital gain.

Same would be true for the SensorExposure control, as we could convey 
the exposure time we want, and it would be converted accordingly into
VBLANK + exposure (and then would deal with VBLANK beeing called before 
calculating the exposure value in lines with the updated maximum).

Thus, VBLANK would not be a control for IPA at all, and we would have 
Gain and ExposureTime (for instance) only ?

I don't think it should be done on this series though, as I said, so 
with or without:
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at yoseli.org>

> +
> +...
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index b57bee7ef6ca..4564d3dbb835 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -99,6 +99,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 +112,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