[libcamera-devel] [PATCH v3 1/5] libcamera: controls: Updates to gain and exposure controls

[Laurent Pinchart]

Hi Naush,

On Thu, Apr 23, 2020 at 08:34:43PM +0300, Laurent Pinchart wrote:
> On Fri, Apr 03, 2020 at 03:53:01PM +0100, Naushir Patuck wrote:
> > Rename:
> > ManualExposure -> ExposureTime
> > ManualGain -> AnalogueGain

I forgot to mention that AwbEnable still references ManualGain. It's
fixed by a patch further in the series, but it's best to fix it here.
I'll do that in my tree, but if you post a new version, could you please
include that fix ?

> > Use micro-seconds units for ExposureTime. This is changed from milli-
> > seconds. The latter would not allow very low exposure times.
> > 
> > AnalogueGain switch to use a float to allow fractional gain adjustments.
> > 
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/libcamera/control_ids.yaml               | 20 ++++++++++++++------
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |  8 ++++----
> >  2 files changed, 18 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 4befec74..839eea76 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -10,7 +10,7 @@ controls:
> >        description: |
> >          Enable or disable the AE.
> >  
> > -        \sa ManualExposure
> > +        \sa ExposureTime AnalogueGain
> >  
> >    - AeLocked:
> >        type: bool
> > @@ -42,12 +42,20 @@ controls:
> >        type: int32_t
> >        description: Specify a fixed saturation parameter
> >  
> > -  - ManualExposure:
> > +  - ExposureTime:
> >        type: int32_t
> > -      description: Specify a fixed exposure time in milli-seconds
> > +      description: |
> > +        Exposure time (shutter speed) for the frame applied in the sensor
> > +        device. This value is specified in micro-seconds.
> >  
> > -  - ManualGain:
> > -      type: int32_t
> > -      description: Specify a fixed gain parameter
> > +        \sa AnalogueGain AeEnable
> > +
> > +  - AnalogueGain:
> > +      type: float
> > +      description: |
> > +        Analogue gain value applied in the sensor device.
> > +        The value of the control specifies the gain multiplier applied to all
> > +        colour channels. This value cannot be lower than 1.0.
> 
> I wonder if there could be use cases for a value lower than 1.0, but
> that can be done later if/when needed, so no issue for now.
> 
> >  
> > +        \sa ExposureTime AeEnable
> >  ...
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index ffbddf27..d7df95e4 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -251,10 +251,10 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> >  			controls.set(V4L2_CID_CONTRAST, value);
> >  		} else if (id == controls::Saturation) {
> >  			controls.set(V4L2_CID_SATURATION, value);
> > -		} else if (id == controls::ManualExposure) {
> > +		} else if (id == controls::ExposureTime) {
> >  			controls.set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));
> >  			controls.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);
> > -		} else if (id == controls::ManualGain) {
> > +		} else if (id == controls::AnalogueGain) {
> >  			controls.set(V4L2_CID_GAIN, value);
> >  		}
> >  	}
> 
> The unit change breaks UVC support, which you then fixed in the next
> patch in the series. We try to avoid bisection breakages when possible,
> so I would like to squash 1/5 and 2/5 together unless there's an
> objection.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> > @@ -364,10 +364,10 @@ int UVCCameraData::init(MediaEntity *entity)
> >  			id = &controls::Saturation;
> >  			break;
> >  		case V4L2_CID_EXPOSURE_ABSOLUTE:
> > -			id = &controls::ManualExposure;
> > +			id = &controls::ExposureTime;
> >  			break;
> >  		case V4L2_CID_GAIN:
> > -			id = &controls::ManualGain;
> > +			id = &controls::AnalogueGain;
> >  			break;
> >  		default:
> >  			continue;

-- 
Regards,

Laurent Pinchart