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

[Jacopo Mondi]

+Kieran +Umang

On Wed, Jun 22, 2022 at 09:41:55AM +0200, Jean-Michel Hautbois wrote:
> Hi Jacopo,
>
> On 22/06/2022 09:06, Jacopo Mondi wrote:
> > Hi Jean-Michel,
> >
> > On Tue, Jun 21, 2022 at 08:20:27PM +0200, Jean-Michel Hautbois wrote:
> > > 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.
> >
> > Ah! I copied the type from the V4L2 control definition!
> >
> > I see your point, but doesn't IPA have access to the CameraSensorHelper class
> > exactly for this purpose (convert the gain value into gain code) ?
> >
> > Now, I would dare sayin that the IPA could be even detached from any
> > sensor-related knowledge and CameraSensorHelpers moved to the
> > PH/CameraSensor side, so that IPAs only calculate the "analogue" value
> > and the CameraSensor class computes the gain code.
> >
> > Is this the direction where we want to go ?
>
> It is the direction I would like it to go, decoupling the IPA from V4L2
> controls. They can't be totally agnostic from the hardware, as they take
> statistics and compute values based on it to send the parameters back, but
> on the AGC side at least, it does not need to know what the driver can do at
> all. It just need to know the min/max it can apply for the gain (and time)
> that's all, I think.
>

Any consideration about the closed-source IPA modules ?