| Message ID | 20220323160145.90606-4-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-23 16:01:44) > The lens focus is controled by a VCM, which is linked to the sensor > using the ancillary links. > Pass the control to the config info structure and make it possible to > update by the IPA. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > include/libcamera/ipa/raspberrypi.mojom | 1 + > .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > index acd3cafe..0c3922b0 100644 > --- a/include/libcamera/ipa/raspberrypi.mojom > +++ b/include/libcamera/ipa/raspberrypi.mojom > @@ -125,4 +125,5 @@ interface IPARPiEventInterface { > embeddedComplete(uint32 bufferId); > setIspControls(libcamera.ControlList controls); > setDelayedControls(libcamera.ControlList controls); > + setLensControls(libcamera.ControlList controls); > }; > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index c2230199..970f9fe7 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -33,6 +33,7 @@ > > #include "libcamera/internal/bayer_format.h" > #include "libcamera/internal/camera.h" > +#include "libcamera/internal/camera_lens.h" > #include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/device_enumerator.h" > @@ -202,6 +203,7 @@ public: > void setIspControls(const ControlList &controls); > void setDelayedControls(const ControlList &controls); > void setSensorControls(ControlList &controls); > + void setLensControls(const ControlList &controls); > > /* bufferComplete signal handlers. */ > void unicamBufferDequeue(FrameBuffer *buffer); > @@ -1483,6 +1485,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) > ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete); > ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); > + ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls); > > /* > * The configuration (tuning file) is made from the sensor name unless > @@ -1519,6 +1522,10 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) > entityControls.emplace(0, sensor_->controls()); > entityControls.emplace(1, isp_[Isp::Input].dev()->controls()); > > + CameraLens *lens = sensor_->focusLens(); > + if (lens) > + entityControls.emplace(2, lens->controls()); > + > /* Always send the user transform to the IPA. */ > ipaConfig.transform = static_cast<unsigned int>(config->transform); > > @@ -1735,6 +1742,16 @@ void RPiCameraData::setDelayedControls(const ControlList &controls) > handleState(); > } > > +void RPiCameraData::setLensControls(const ControlList &ctrls) > +{ > + CameraLens *lens = sensor_->focusLens(); > + if (!lens) > + return; > + > + /* \todo Should we keep track of the latest value applied ? */ > + lens->setFocusPosition(ctrls.get(V4L2_CID_FOCUS_ABSOLUTE).get<int32_t>()); Does the algorithm currently take into account what position the lens is at for the statistics it is considering? I'm concerned that it isn't, or that it's considering the position / top of the hill as a value which is mis-matched against the frame...? I presume there are delays that mean it might even be hard to determine how long it takes to move the VCM to a given position - so that might make it more difficult to state which frame had which position? > +} > + > void RPiCameraData::setSensorControls(ControlList &controls) > { > /* > -- > 2.32.0 >
Hi Kieran, On 24/03/2022 00:54, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-23 16:01:44) >> The lens focus is controled by a VCM, which is linked to the sensor >> using the ancillary links. >> Pass the control to the config info structure and make it possible to >> update by the IPA. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> include/libcamera/ipa/raspberrypi.mojom | 1 + >> .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++++++++++++ >> 2 files changed, 18 insertions(+) >> >> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom >> index acd3cafe..0c3922b0 100644 >> --- a/include/libcamera/ipa/raspberrypi.mojom >> +++ b/include/libcamera/ipa/raspberrypi.mojom >> @@ -125,4 +125,5 @@ interface IPARPiEventInterface { >> embeddedComplete(uint32 bufferId); >> setIspControls(libcamera.ControlList controls); >> setDelayedControls(libcamera.ControlList controls); >> + setLensControls(libcamera.ControlList controls); >> }; >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> index c2230199..970f9fe7 100644 >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >> @@ -33,6 +33,7 @@ >> >> #include "libcamera/internal/bayer_format.h" >> #include "libcamera/internal/camera.h" >> +#include "libcamera/internal/camera_lens.h" >> #include "libcamera/internal/camera_sensor.h" >> #include "libcamera/internal/delayed_controls.h" >> #include "libcamera/internal/device_enumerator.h" >> @@ -202,6 +203,7 @@ public: >> void setIspControls(const ControlList &controls); >> void setDelayedControls(const ControlList &controls); >> void setSensorControls(ControlList &controls); >> + void setLensControls(const ControlList &controls); >> >> /* bufferComplete signal handlers. */ >> void unicamBufferDequeue(FrameBuffer *buffer); >> @@ -1483,6 +1485,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) >> ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete); >> ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); >> ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); >> + ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls); >> >> /* >> * The configuration (tuning file) is made from the sensor name unless >> @@ -1519,6 +1522,10 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) >> entityControls.emplace(0, sensor_->controls()); >> entityControls.emplace(1, isp_[Isp::Input].dev()->controls()); >> >> + CameraLens *lens = sensor_->focusLens(); >> + if (lens) >> + entityControls.emplace(2, lens->controls()); >> + >> /* Always send the user transform to the IPA. */ >> ipaConfig.transform = static_cast<unsigned int>(config->transform); >> >> @@ -1735,6 +1742,16 @@ void RPiCameraData::setDelayedControls(const ControlList &controls) >> handleState(); >> } >> >> +void RPiCameraData::setLensControls(const ControlList &ctrls) >> +{ >> + CameraLens *lens = sensor_->focusLens(); >> + if (!lens) >> + return; >> + >> + /* \todo Should we keep track of the latest value applied ? */ >> + lens->setFocusPosition(ctrls.get(V4L2_CID_FOCUS_ABSOLUTE).get<int32_t>()); > > Does the algorithm currently take into account what position the lens is > at for the statistics it is considering? I'm concerned that it isn't, or > that it's considering the position / top of the hill as a value which is > mis-matched against the frame...? > > I presume there are delays that mean it might even be hard to determine > how long it takes to move the VCM to a given position - so that might > make it more difficult to state which frame had which position? That's a good remark, and I don't really have an answer to this yet. I suppose there is a delay, but I don't know how to measure it easily. And I am not sure it is a very big one, so it could very well not be a big issue (opened question !) ? > > >> +} >> + >> void RPiCameraData::setSensorControls(ControlList &controls) >> { >> /* >> -- >> 2.32.0 >>
Hi, On Thu, 24 Mar 2022 at 08:22, Jean-Michel Hautbois via libcamera-devel < libcamera-devel@lists.libcamera.org> wrote: > Hi Kieran, > > On 24/03/2022 00:54, Kieran Bingham wrote: > > Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-23 16:01:44) > >> The lens focus is controled by a VCM, which is linked to the sensor > >> using the ancillary links. > >> Pass the control to the config info structure and make it possible to > >> update by the IPA. > >> > >> Signed-off-by: Jean-Michel Hautbois < > jeanmichel.hautbois@ideasonboard.com> > >> --- > >> include/libcamera/ipa/raspberrypi.mojom | 1 + > >> .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++++++++++++ > >> 2 files changed, 18 insertions(+) > >> > >> diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ipa/raspberrypi.mojom > >> index acd3cafe..0c3922b0 100644 > >> --- a/include/libcamera/ipa/raspberrypi.mojom > >> +++ b/include/libcamera/ipa/raspberrypi.mojom > >> @@ -125,4 +125,5 @@ interface IPARPiEventInterface { > >> embeddedComplete(uint32 bufferId); > >> setIspControls(libcamera.ControlList controls); > >> setDelayedControls(libcamera.ControlList controls); > >> + setLensControls(libcamera.ControlList controls); > >> }; > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> index c2230199..970f9fe7 100644 > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >> @@ -33,6 +33,7 @@ > >> > >> #include "libcamera/internal/bayer_format.h" > >> #include "libcamera/internal/camera.h" > >> +#include "libcamera/internal/camera_lens.h" > >> #include "libcamera/internal/camera_sensor.h" > >> #include "libcamera/internal/delayed_controls.h" > >> #include "libcamera/internal/device_enumerator.h" > >> @@ -202,6 +203,7 @@ public: > >> void setIspControls(const ControlList &controls); > >> void setDelayedControls(const ControlList &controls); > >> void setSensorControls(ControlList &controls); > >> + void setLensControls(const ControlList &controls); > >> > >> /* bufferComplete signal handlers. */ > >> void unicamBufferDequeue(FrameBuffer *buffer); > >> @@ -1483,6 +1485,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig > *sensorConfig) > >> ipa_->embeddedComplete.connect(this, > &RPiCameraData::embeddedComplete); > >> ipa_->setIspControls.connect(this, > &RPiCameraData::setIspControls); > >> ipa_->setDelayedControls.connect(this, > &RPiCameraData::setDelayedControls); > >> + ipa_->setLensControls.connect(this, > &RPiCameraData::setLensControls); > >> > >> /* > >> * The configuration (tuning file) is made from the sensor > name unless > >> @@ -1519,6 +1522,10 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > >> entityControls.emplace(0, sensor_->controls()); > >> entityControls.emplace(1, isp_[Isp::Input].dev()->controls()); > >> > >> + CameraLens *lens = sensor_->focusLens(); > >> + if (lens) > >> + entityControls.emplace(2, lens->controls()); > >> + > >> /* Always send the user transform to the IPA. */ > >> ipaConfig.transform = static_cast<unsigned > int>(config->transform); > >> > >> @@ -1735,6 +1742,16 @@ void RPiCameraData::setDelayedControls(const > ControlList &controls) > >> handleState(); > >> } > >> > >> +void RPiCameraData::setLensControls(const ControlList &ctrls) > >> +{ > >> + CameraLens *lens = sensor_->focusLens(); > >> + if (!lens) > >> + return; > >> + > >> + /* \todo Should we keep track of the latest value applied ? */ > >> + > lens->setFocusPosition(ctrls.get(V4L2_CID_FOCUS_ABSOLUTE).get<int32_t>()); > > > > Does the algorithm currently take into account what position the lens is > > at for the statistics it is considering? I'm concerned that it isn't, or > > that it's considering the position / top of the hill as a value which is > > mis-matched against the frame...? > > > > I presume there are delays that mean it might even be hard to determine > > how long it takes to move the VCM to a given position - so that might > > make it more difficult to state which frame had which position? > > That's a good remark, and I don't really have an answer to this yet. I > suppose there is a delay, but I don't know how to measure it easily. And > I am not sure it is a very big one, so it could very well not be a big > issue (opened question !) ? > This is quite a tricky thing to do. One option is have the af algorithm count time/frames and assume it knows about the characteristics of the actuator and knows when the lens stopped moving to sample the stats. Another option would be to use the DelayedControls to return the "correct" lens position for each frame. The problem with this is the lens movement time is dependent on displacement needed. So, we need to adapt DelayedControls to allow something like "set the control now but inform me of the change N frames later." That would work, and keeps in line with how delayed controls report shutter/gain from the sensor. Naush > > > > > > >> +} > >> + > >> void RPiCameraData::setSensorControls(ControlList &controls) > >> { > >> /* > >> -- > >> 2.32.0 > >> >
On Thu, Mar 24, 2022 at 10:28:33AM +0000, Naushir Patuck wrote: > On Thu, 24 Mar 2022 at 08:22, Jean-Michel Hautbois wrote: > > On 24/03/2022 00:54, Kieran Bingham wrote: > > > Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-23 16:01:44) > > >> The lens focus is controled by a VCM, which is linked to the sensor > > >> using the ancillary links. > > >> Pass the control to the config info structure and make it possible to > > >> update by the IPA. > > >> > > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > >> --- > > >> include/libcamera/ipa/raspberrypi.mojom | 1 + > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++++++++++++ > > >> 2 files changed, 18 insertions(+) > > >> > > >> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom > > >> index acd3cafe..0c3922b0 100644 > > >> --- a/include/libcamera/ipa/raspberrypi.mojom > > >> +++ b/include/libcamera/ipa/raspberrypi.mojom > > >> @@ -125,4 +125,5 @@ interface IPARPiEventInterface { > > >> embeddedComplete(uint32 bufferId); > > >> setIspControls(libcamera.ControlList controls); > > >> setDelayedControls(libcamera.ControlList controls); > > >> + setLensControls(libcamera.ControlList controls); > > >> }; > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >> index c2230199..970f9fe7 100644 > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >> @@ -33,6 +33,7 @@ > > >> > > >> #include "libcamera/internal/bayer_format.h" > > >> #include "libcamera/internal/camera.h" > > >> +#include "libcamera/internal/camera_lens.h" > > >> #include "libcamera/internal/camera_sensor.h" > > >> #include "libcamera/internal/delayed_controls.h" > > >> #include "libcamera/internal/device_enumerator.h" > > >> @@ -202,6 +203,7 @@ public: > > >> void setIspControls(const ControlList &controls); > > >> void setDelayedControls(const ControlList &controls); > > >> void setSensorControls(ControlList &controls); > > >> + void setLensControls(const ControlList &controls); > > >> > > >> /* bufferComplete signal handlers. */ > > >> void unicamBufferDequeue(FrameBuffer *buffer); > > >> @@ -1483,6 +1485,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) > > >> ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete); > > >> ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); > > >> ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); > > >> + ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls); > > >> > > >> /* > > >> * The configuration (tuning file) is made from the sensor name unless > > >> @@ -1519,6 +1522,10 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) > > >> entityControls.emplace(0, sensor_->controls()); > > >> entityControls.emplace(1, isp_[Isp::Input].dev()->controls()); > > >> > > >> + CameraLens *lens = sensor_->focusLens(); > > >> + if (lens) > > >> + entityControls.emplace(2, lens->controls()); > > >> + > > >> /* Always send the user transform to the IPA. */ > > >> ipaConfig.transform = static_cast<unsigned int>(config->transform); > > >> > > >> @@ -1735,6 +1742,16 @@ void RPiCameraData::setDelayedControls(const ControlList &controls) > > >> handleState(); > > >> } > > >> > > >> +void RPiCameraData::setLensControls(const ControlList &ctrls) > > >> +{ > > >> + CameraLens *lens = sensor_->focusLens(); > > >> + if (!lens) > > >> + return; > > >> + > > >> + /* \todo Should we keep track of the latest value applied ? */ > > >> + lens->setFocusPosition(ctrls.get(V4L2_CID_FOCUS_ABSOLUTE).get<int32_t>()); > > > > > > Does the algorithm currently take into account what position the lens is > > > at for the statistics it is considering? I'm concerned that it isn't, or > > > that it's considering the position / top of the hill as a value which is > > > mis-matched against the frame...? > > > > > > I presume there are delays that mean it might even be hard to determine > > > how long it takes to move the VCM to a given position - so that might > > > make it more difficult to state which frame had which position? > > > > That's a good remark, and I don't really have an answer to this yet. I > > suppose there is a delay, but I don't know how to measure it easily. And > > I am not sure it is a very big one, so it could very well not be a big > > issue (opened question !) ? > > This is quite a tricky thing to do. One option is have the af algorithm > count time/frames and assume it knows about the characteristics of the > actuator and knows when the lens stopped moving to sample the stats. I think we'll need that. VCM actuators can be very tricky, especially when they don't have a control loop. Large swing values will result in overshoot and ringing. Some open-loop VCM actuators have hardware features to generate a ramp instead of a step, which can already help (the ramp parameters can be programmable). > Another option would be to use the DelayedControls to return the "correct" > lens position for each frame. The problem with this is the lens movement > time is dependent on displacement needed. So, we need to adapt > DelayedControls to allow something like "set the control now but inform me > of the change N frames later." That would work, and keeps in line with how > delayed controls report shutter/gain from the sensor. I don't think DelayedControl should handle this, it will be very VCM-specific. A VCM helper (or rather, most likely, VCM helper*s*) would be better. > > >> +} > > >> + > > >> void RPiCameraData::setSensorControls(ControlList &controls) > > >> { > > >> /*
Hi, On Sun, 27 Mar 2022 at 23:01, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On Thu, Mar 24, 2022 at 10:28:33AM +0000, Naushir Patuck wrote: > > On Thu, 24 Mar 2022 at 08:22, Jean-Michel Hautbois wrote: > > > On 24/03/2022 00:54, Kieran Bingham wrote: > > > > Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-23 > 16:01:44) > > > >> The lens focus is controled by a VCM, which is linked to the sensor > > > >> using the ancillary links. > > > >> Pass the control to the config info structure and make it possible > to > > > >> update by the IPA. > > > >> > > > >> Signed-off-by: Jean-Michel Hautbois < > jeanmichel.hautbois@ideasonboard.com> > > > >> --- > > > >> include/libcamera/ipa/raspberrypi.mojom | 1 + > > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 17 > +++++++++++++++++ > > > >> 2 files changed, 18 insertions(+) > > > >> > > > >> diff --git a/include/libcamera/ipa/raspberrypi.mojom > b/include/libcamera/ipa/raspberrypi.mojom > > > >> index acd3cafe..0c3922b0 100644 > > > >> --- a/include/libcamera/ipa/raspberrypi.mojom > > > >> +++ b/include/libcamera/ipa/raspberrypi.mojom > > > >> @@ -125,4 +125,5 @@ interface IPARPiEventInterface { > > > >> embeddedComplete(uint32 bufferId); > > > >> setIspControls(libcamera.ControlList controls); > > > >> setDelayedControls(libcamera.ControlList controls); > > > >> + setLensControls(libcamera.ControlList controls); > > > >> }; > > > >> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > >> index c2230199..970f9fe7 100644 > > > >> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > >> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > >> @@ -33,6 +33,7 @@ > > > >> > > > >> #include "libcamera/internal/bayer_format.h" > > > >> #include "libcamera/internal/camera.h" > > > >> +#include "libcamera/internal/camera_lens.h" > > > >> #include "libcamera/internal/camera_sensor.h" > > > >> #include "libcamera/internal/delayed_controls.h" > > > >> #include "libcamera/internal/device_enumerator.h" > > > >> @@ -202,6 +203,7 @@ public: > > > >> void setIspControls(const ControlList &controls); > > > >> void setDelayedControls(const ControlList &controls); > > > >> void setSensorControls(ControlList &controls); > > > >> + void setLensControls(const ControlList &controls); > > > >> > > > >> /* bufferComplete signal handlers. */ > > > >> void unicamBufferDequeue(FrameBuffer *buffer); > > > >> @@ -1483,6 +1485,7 @@ int > RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) > > > >> ipa_->embeddedComplete.connect(this, > &RPiCameraData::embeddedComplete); > > > >> ipa_->setIspControls.connect(this, > &RPiCameraData::setIspControls); > > > >> ipa_->setDelayedControls.connect(this, > &RPiCameraData::setDelayedControls); > > > >> + ipa_->setLensControls.connect(this, > &RPiCameraData::setLensControls); > > > >> > > > >> /* > > > >> * The configuration (tuning file) is made from the sensor > name unless > > > >> @@ -1519,6 +1522,10 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *config) > > > >> entityControls.emplace(0, sensor_->controls()); > > > >> entityControls.emplace(1, > isp_[Isp::Input].dev()->controls()); > > > >> > > > >> + CameraLens *lens = sensor_->focusLens(); > > > >> + if (lens) > > > >> + entityControls.emplace(2, lens->controls()); > > > >> + > > > >> /* Always send the user transform to the IPA. */ > > > >> ipaConfig.transform = static_cast<unsigned > int>(config->transform); > > > >> > > > >> @@ -1735,6 +1742,16 @@ void RPiCameraData::setDelayedControls(const > ControlList &controls) > > > >> handleState(); > > > >> } > > > >> > > > >> +void RPiCameraData::setLensControls(const ControlList &ctrls) > > > >> +{ > > > >> + CameraLens *lens = sensor_->focusLens(); > > > >> + if (!lens) > > > >> + return; > > > >> + > > > >> + /* \todo Should we keep track of the latest value applied ? > */ > > > >> + > lens->setFocusPosition(ctrls.get(V4L2_CID_FOCUS_ABSOLUTE).get<int32_t>()); > > > > > > > > Does the algorithm currently take into account what position the > lens is > > > > at for the statistics it is considering? I'm concerned that it > isn't, or > > > > that it's considering the position / top of the hill as a value > which is > > > > mis-matched against the frame...? > > > > > > > > I presume there are delays that mean it might even be hard to > determine > > > > how long it takes to move the VCM to a given position - so that might > > > > make it more difficult to state which frame had which position? > > > > > > That's a good remark, and I don't really have an answer to this yet. I > > > suppose there is a delay, but I don't know how to measure it easily. > And > > > I am not sure it is a very big one, so it could very well not be a big > > > issue (opened question !) ? > > > > This is quite a tricky thing to do. One option is have the af algorithm > > count time/frames and assume it knows about the characteristics of the > > actuator and knows when the lens stopped moving to sample the stats. > > I think we'll need that. VCM actuators can be very tricky, especially > when they don't have a control loop. Large swing values will result in > overshoot and ringing. Some open-loop VCM actuators have hardware > features to generate a ramp instead of a step, which can already help > (the ramp parameters can be programmable). > > > Another option would be to use the DelayedControls to return the > "correct" > > lens position for each frame. The problem with this is the lens movement > > time is dependent on displacement needed. So, we need to adapt > > DelayedControls to allow something like "set the control now but inform > me > > of the change N frames later." That would work, and keeps in line with > how > > delayed controls report shutter/gain from the sensor. > > I don't think DelayedControl should handle this, it will be very > VCM-specific. A VCM helper (or rather, most likely, VCM helper*s*) would > be better. > I agree that DelayedControls is not the right place for this. Perhaps a new member function CameraLens::getrFocusPosition that takes in the frame sequence number and returns the expected position through some dead-reckoning calculations? Regards, Naush > > > > >> +} > > > >> + > > > >> void RPiCameraData::setSensorControls(ControlList &controls) > > > >> { > > > >> /* > > -- > Regards, > > Laurent Pinchart >
diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom index acd3cafe..0c3922b0 100644 --- a/include/libcamera/ipa/raspberrypi.mojom +++ b/include/libcamera/ipa/raspberrypi.mojom @@ -125,4 +125,5 @@ interface IPARPiEventInterface { embeddedComplete(uint32 bufferId); setIspControls(libcamera.ControlList controls); setDelayedControls(libcamera.ControlList controls); + setLensControls(libcamera.ControlList controls); }; diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index c2230199..970f9fe7 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -33,6 +33,7 @@ #include "libcamera/internal/bayer_format.h" #include "libcamera/internal/camera.h" +#include "libcamera/internal/camera_lens.h" #include "libcamera/internal/camera_sensor.h" #include "libcamera/internal/delayed_controls.h" #include "libcamera/internal/device_enumerator.h" @@ -202,6 +203,7 @@ public: void setIspControls(const ControlList &controls); void setDelayedControls(const ControlList &controls); void setSensorControls(ControlList &controls); + void setLensControls(const ControlList &controls); /* bufferComplete signal handlers. */ void unicamBufferDequeue(FrameBuffer *buffer); @@ -1483,6 +1485,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig) ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete); ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls); ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls); + ipa_->setLensControls.connect(this, &RPiCameraData::setLensControls); /* * The configuration (tuning file) is made from the sensor name unless @@ -1519,6 +1522,10 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) entityControls.emplace(0, sensor_->controls()); entityControls.emplace(1, isp_[Isp::Input].dev()->controls()); + CameraLens *lens = sensor_->focusLens(); + if (lens) + entityControls.emplace(2, lens->controls()); + /* Always send the user transform to the IPA. */ ipaConfig.transform = static_cast<unsigned int>(config->transform); @@ -1735,6 +1742,16 @@ void RPiCameraData::setDelayedControls(const ControlList &controls) handleState(); } +void RPiCameraData::setLensControls(const ControlList &ctrls) +{ + CameraLens *lens = sensor_->focusLens(); + if (!lens) + return; + + /* \todo Should we keep track of the latest value applied ? */ + lens->setFocusPosition(ctrls.get(V4L2_CID_FOCUS_ABSOLUTE).get<int32_t>()); +} + void RPiCameraData::setSensorControls(ControlList &controls) { /*
The lens focus is controled by a VCM, which is linked to the sensor using the ancillary links. Pass the control to the config info structure and make it possible to update by the IPA. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- include/libcamera/ipa/raspberrypi.mojom | 1 + .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++++++++++++++ 2 files changed, 18 insertions(+)