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

[Laurent Pinchart]

Hi Naush,

On Fri, Mar 27, 2020 at 10:59:56AM +0000, Naushir Patuck wrote:
> On Thu, 26 Mar 2020 at 16:34, Laurent Pinchart wrote:
> > 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.

Should we then rename ManualGain to AnalogGain ?

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