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