[libcamera-devel] [PATCH v2 2/6] libcamera: uvcvideo: Update exposure/gain ctrls set with new units

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Mon, Mar 23, 2020 at 04:03:57PM +0000, Naushir Patuck wrote:
> On Fri, 20 Mar 2020 at 14:28, Kieran Bingham wrote:
> > On 09/03/2020 12:33, Naushir Patuck wrote:
> > > The ManualExposure control now uses units of 1 micro-second, and UVC
> > > uses units of 100 micro-seconds. Correctly map the values before
> > > setting V4L2_CID_EXPOSURE_ABSOLUTE on the V4L2 device.
> > >
> > > The ManualGain control now uses floats to allow fractional gain values.
> > > Since UVC has no explicit gain units, map the default gain value to 1.0
> > > and linearly map to the requested value.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/uvcvideo.cpp | 17 +++++++++++++++--
> > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > > index 29afb121..aff86803 100644
> > > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > > @@ -245,9 +245,22 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > >                       controls.set(V4L2_CID_SATURATION, value);
> > >               } else if (id == controls::ManualExposure) {
> > >                       controls.set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));
> > > -                     controls.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);
> > > +                     /*
> > > +                      * controls::ManualExposure is in units of 1 us, and UVC
> > > +                      * expects V4L2_CID_EXPOSURE_ABSOLUTE in units of 100 us.
> > > +                      * So divide by 100 when setting the control.
> >
> > Sounds fine, but possibly the last line isn't needed, or I'd at least
> > remove the 'So' ... but that's trivial/minor.
> 
> Will do.
> 
> > > +                      */
> > > +                     controls.set(V4L2_CID_EXPOSURE_ABSOLUTE, value.get<int32_t>() / 100);
> > >               } else if (id == controls::ManualGain) {
> > > -                     controls.set(V4L2_CID_GAIN, value);
> > > +                     /*
> > > +                      * controls::ManualGain is specified as an absolute float value.
> > > +                      * Map this in a linear way such that 1.0 -> default gain
> > > +                      * of the V4L2_CID_GAIN control.
> >
> > This doesn't seem very clear to me yet. (I get the intent it's just the
> > description), and if we're 'mapping' the value like this should it be
> > documented at the control level?
> >
> > Or do you expect this to apply only to UVC? (Presumably if a device
> > doesn't specify a default gain, it's just '1.0' ?)
> 
> This mapping is explicitly for UVC.
> 
> The ManualExposure control is meant to be an explicit exposure time to
> use, and likewise, ManualGain is an explicit gain multiplier applied
> to all samples.  From Laurent's comment on patch v1, he noted that UVC
> doesn't specify units for gain.  I took that to mean that the gain
> control is just a range-type control to increase the brightness, and
> the mapping in the uvcvideo pipeline is to sanitise the control usage
> in this case.
> 
> > Perhaps going back to patch 1/6 we need to explain more about how
> > ManualGain is represented to document it's usage clearly there...
> 
> I can certainly be more descriptive in the description of the control if needed.
> 
> > > +                      */
> > > +                     ControlRange gainInfo = controls.infoMap()->at(V4L2_CID_GAIN);

You can make this a

			    const ControlRange &gainInfo = controls.infoMap()->at(V4L2_CID_GAIN);

to avoid making a copy.

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > > +                     float requestGain = value.get<float>();
> > > +                     int32_t gain = requestGain * gainInfo.def().get<int32_t>();

Minor comment, you could possible use

			int32_t gain = value.get<float>()
				     * gainInfo.def().get<int32_t>();

to avoid the requestGain variable.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > > +                     controls.set(V4L2_CID_GAIN, gain);
> > >               }
> > >       }
> > >

-- 
Regards,

Laurent Pinchart