| Message ID | 20200403145305.10288-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Fri, Apr 03, 2020 at 03:53:01PM +0100, Naushir Patuck wrote: > Rename: > ManualExposure -> ExposureTime > ManualGain -> AnalogueGain > > 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@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@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;
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@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@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;
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. + \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); } } @@ -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;
Rename: ManualExposure -> ExposureTime ManualGain -> AnalogueGain 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@raspberrypi.com> --- src/libcamera/control_ids.yaml | 20 ++++++++++++++------ src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 8 ++++---- 2 files changed, 18 insertions(+), 10 deletions(-)