[libcamera-devel] [PATCH v2 6/6] libcamera: controls: Add controls to report back frame metadata

[Naushir Patuck]

Hi Laurent,

On Thu, 26 Mar 2020 at 16:34, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hello,
>
> On Fri, Mar 20, 2020 at 04:25:49PM +0000, David Plowman wrote:
> > Yes, we return the colour temperature in Kelvin. But you're right, and
> > I think there's a general point here. How is code going to get less
> > platform specific if we don't define what all these numbers mean, by
> > which I mean both the controls that you set and the values that may
> > get reported back?
>
> I think defining the units and ranges is the best approach when
> possible. Controls with custom units should be an exception. For ranges,
> they could certainly be platform specific for some controls, but that's
> less of an issue if the units are well-defined.
>
> > On Fri, 20 Mar 2020 at 15:45, Kieran Bingham wrote:
> > > On 09/03/2020 12:33, Naushir Patuck wrote:
> > > > Add controls to report the current frame's exposure and gain, and an
> > > > estimate of the current lux level from the AE algorithm.
> > > >
> > > > Add controls to report the red and blue colour gains, and an estimate of
> > > > the colour temperature from the AWB algorithm.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > ---
> > > >  src/libcamera/control_ids.yaml | 28 ++++++++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > index 3d1b058f..ee93975e 100644
> > > > --- a/src/libcamera/control_ids.yaml
> > > > +++ b/src/libcamera/control_ids.yaml
> > > > @@ -138,6 +138,22 @@ controls:
> > > >
> > > >          \sa ManualExposure
> > > >
> > > > +  - CurrentExposure:
> > > > +      type: int32_t
> > > > +      description: Report the exposure time in micro-seconds of this frame.
> > > > +
> > > > +        \sa CurrentAnalogueGain
> > > > +
>
> I don't think we need a separate control for this. We have
> ManualExposure already (which I think we'll rename to ExposureTime), and
> you can use it in metadata. It's best to use the same control for the
> request control and metadata when they relate to the same value.
>

That seems like a good option.  We can rename ManualExposure to
ExposureTime and reuse for the return metadata.

> > > > +  - CurrentAnalogueGain:
> > > > +      type: float
> > > > +      description: Report the analogue gain parameter for the current frame.
> > > > +
> > > > +        \sa CurrentExposure
>
> Does this one correspond to any of the other controls ?

CurrentAnalogueGain has the corresponding control ManualGain (which we
are having a separate discussion about) so we should be able to reuse
that control.

>
> > > > +
> > > > +  - CurrentLux:
> > > > +      type: float
> > > > +      description: Report an estimate of the current lux level.
>
> I don't think we need the 'Current' prefix. Reporting a control in the
> request metadata implies it's the current value.

Will remove the Current prefix.

>
> > > > +
> > > >    - AwbEnable:
> > > >        type: bool
> > > >        description: |
> > > > @@ -190,6 +206,18 @@ controls:
> > > >          in that order.
> > > >        size: [2]
> > > >
> > > > +  - CurrentWbGains:
> > > > +      type: float
> > > > +      description: |
> > > > +        Report the current gain AWB gain values for the Red and Blue colour
> > > > +        channels, in that order.
> > > > +      size: [2]
>
> For this one we already have a control.
>
> > > > +
> > > > +  - CurrentTemperature:
> > > > +      type: float
> > > > +      description:  |
> > > > +        Report the current estimate of the colour temperature for this frame.
> > >
> > > Do we need to explicitly define the scale/units of the temperature?
> > > (Presumably people would assume kelvins, but perhaps people could
> > > misinterpret this somehow if we're not explicit)
>
> And here we can drop the Current prefix too. I'd call it
> ColorTemperature (or ColourTemperature ?) though, to avoid confusion
> with the temperature of the camera sensor that we may decide to report
> later on.

Will do.

> > >
> > > > +
> > > >    - Brightness:
> > > >        type: int32_t
> > > >        description: |
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Regards,
Naush