[libcamera-devel] [PATCH 2/3] libcamera: controls: Specify manual gain units and change exposure units

[Naushir Patuck]

Hi,

On Tue, 18 Feb 2020 at 00:45, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hello,
>
> On Mon, Feb 17, 2020 at 04:21:32PM +0000, Kieran Bingham wrote:
> > On 17/02/2020 14:26, Naushir Patuck wrote:
> > > Use micro-seconds for ManualExposure. This is changed from milli-
> > > seconds. The latter would not allow very low exposure times.
> > >
> > > Use double for ManualGain to allow fractional gain adjustments.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  src/libcamera/control_ids.yaml | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > index 4befec7..33062d6 100644
> > > --- a/src/libcamera/control_ids.yaml
> > > +++ b/src/libcamera/control_ids.yaml
> > > @@ -44,10 +44,10 @@ controls:
> > >
> > >    - ManualExposure:
> > >        type: int32_t
> > > -      description: Specify a fixed exposure time in milli-seconds
> > > +      description: Specify a fixed exposure time in micro-seconds
> >
> > At the moment, the only user of this control
> > (PipelineHandlerUVC::processControls()) sets this directly on the V4L2
> > control.
> >
> > As we don't yet have anything much actually testing this, it's fairly
> > arbitrary - so I'm certainly not against changing the units.
> >
> > But we may need to consider how to map this directly at the platform.
> >
> > That said - that's a job for the pipeline handlers ...
> >
> > But perhaps we need to divide the value by 1000 in the current code at
> > pipeline/uvcvideo.cpp to maintain consistency.
>
> UVC expresses the exposure time in units of 100µs (I mentioned 100ns
> earlier on IRC, that was a mistake, it's the unit for UVC frame
> intervals, not exposure time), so both would be wrong :-) We sure need
> to fix the UVC pipeline handler, but it doesn't need to be part of this
> patch.
>

Agreed, my V2 patchset will have a commit to address both exposure and gain.

> > >    - ManualGain:
> > > -      type: int32_t
> > > -      description: Specify a fixed gain parameter
> > > +      type: double
> > > +      description: Specify a fixed gain parameter.
> >
> > Same comment applies here that if this is changed, the usage in
> > pipeline/uvcvideo.cpp needs to be updated at the same time, so I think
> > we'll need to split this patch out to change the units and fix the
> > (current) implementation for each of the controls one control at a time.
>
> UVC doesn't specify units for gain. One option would be to map the
> default value to 1.0 and consider the gain range to be linear.
>
> > But Otherwise, I agree on the unit changes.
>
> --
> Regards,
>
> Laurent Pinchart

Regards,
Naush