| Message ID | 20200217142609.22837-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, 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@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. > - 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. But Otherwise, I agree on the unit changes.
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@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. > > - 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.
Hi, On Tue, 18 Feb 2020 at 00:45, Laurent Pinchart <laurent.pinchart@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@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
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 - ManualGain: - type: int32_t - description: Specify a fixed gain parameter + type: double + description: Specify a fixed gain parameter. ...
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@raspberrypi.com> --- src/libcamera/control_ids.yaml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)