<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 9 May 2024 at 12:02, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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">Quoting Laurent Pinchart (2024-05-09 11:23:14)<br>
> Hi Naush,<br>
> <br>
> On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote:<br>
> > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote:<br>
> > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote:<br>
> > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote:<br>
> > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote:<br>
> > > > > > The maximum shutter speed calculation in the cam-helper relied on<br>
> > > > > > the frame duration limits being correctly set in the cam-helper's mode<br>
> > > > > > structure. This was not the case on first startup, so the maximum<br>
> > > > > > shutter speed reported back via the ControlInfo was incorrect.<br>
> > > > > ><br>
> > > > > > Fix this by setting up the camera mode in the cam-helper before querying<br>
> > > > > > for the max shutter value.<br>
> > > > > ><br>
> > > > > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > > > > ---<br>
> > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++<br>
> > > > > > 1 file changed, 6 insertions(+)<br>
> > > > > ><br>
> > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp<br>
> > > > > > index 149a133ab662..1d12262bda01 100644<br>
> > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp<br>
> > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp<br>
> > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)<br>
> > > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>());<br>
> > > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>());<br>
> > > > > ><br>
> > > > > > + /*<br>
> > > > > > + * We need to give the helper the min/max frame durations so it can calculate<br>
> > > > > > + * the correct exposure limits below.<br>
> > > > > > + */<br>
> > > > > > + helper_->setCameraMode(mode_);<br>
> > > > > > +<br>
> > > > ><br>
> > > > > Don't you end up doing this twice, once here, and once in<br>
> > > > > IpaBase::configure(), which calls IpaBase::setMode() ?<br>
> > > ><br>
> > > > Yes, it does happen twice. The other option would be to pass a const<br>
> > > > reference to mode_ into the helper through setMode(), allowing us only do<br>
> > > > it once.<br>
> > ><br>
> > > I don't think I follow you.<br>
> > <br>
> > What I mean is that we don't give the cam helper its own copy of the<br>
> > mode structure, but pass it a const reference to the IPA's mode<br>
> > structure. This would solve the problem with the calculation here.<br>
> <br>
> It's probably me, but I still don't get it :-) We have<br>
> <br>
> void CamHelper::setCameraMode(const CameraMode &mode)<br>
> {<br>
> mode_ = mode;<br>
> if (parser_) {<br>
> parser_->reset();<br>
> parser_->setBitsPerPixel(mode.bitdepth);<br>
> parser_->setLineLengthBytes(0); /* We use SetBufferSize. */<br>
> }<br>
> }<br>
> <br>
> called in IpaBase::configure(). This patch adds another call in<br>
> IpaBase::setMode(). You wrote<br>
> <br>
> > The other option would be to pass a const reference to mode_ into the<br>
> > helper through setMode()<br>
> <br>
> Doesn't this patch does exactly that ? How is it another option ?<br>
> <br>
> /me is confused<br>
<br>
<br>
I can see the duplication.<br>
<br>
I guess the question is ... Naush - do you want to remove the second<br>
call? I don't see any need to change the constness of it currently?<br>
<br>
<br>
The call flow I see is:<br>
<br>
IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,<br>
ConfigResult *result) <br>
{<br>
...<br>
/* Re-assemble camera mode using the sensor info. */<br>
setMode(sensorInfo);<br>
-><br>
IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)<br>
{ /* KB: Which with this patch calls */<br>
...<br>
mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>());<br>
mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>());<br>
<br>
+ /*<br>
+ * We need to give the helper the min/max frame durations so it can calculate<br>
+ * the correct exposure limits below.<br>
+ */<br>
+ helper_->setCameraMode(mode_);<br>
<br>
/* Shutter speed is calculated based on the limits of the frame durations. */<br>
mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength);<br>
mode_.maxShutter = Duration::max();<br>
helper_->getBlanking(mode_.maxShutter,<br>
mode_.minFrameDuration, mode_.maxFrameDuration);<br>
...<br>
}<br>
<br>
/* KB: And then flows directly into the next two lines to call<br>
* helper_->setCameraMode(mode_); again */<br>
<br>
mode_.transform = static_cast<libcamera::Transform>(params.transform);<br>
-<br>
- /* Pass the camera mode to the CamHelper to setup algorithms. */<br>
- helper_->setCameraMode(mode_);<br>
<br>
...<br>
}<br>
<br>
So I think the open question is simply, should this patch remove the<br>
lines I've prefixed with '-' as it has added the lines prefixed with<br>
'+'....<br>
<br>
I think doing so require setting the mode_.transform before calling<br>
setMode(sensorInfo), but that looks like it's all it would do?<br></blockquote><div><br></div><div>Almost but not quite. The copy in the helper will not have the updated min/max shutter.</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>
Naush - if you're happy with the patch as is, I can merge it already.<br>
There's probably some other interactions we've missed that you have<br>
better visibility on.<br></blockquote><div><br></div><div>I would suggest sticking with the current patch. It can get fiddly quite quickly moving these bits of logic around.</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>
Kieran<br>
<br>
<br>
<br>
> <br>
> > > > But that was a bit more involved over this more trivial fix. I<br>
> > > > can change to do that if folks prefer.<br>
> > > ><br>
> > > > > > /* Shutter speed is calculated based on the limits of the frame durations. */<br>
> > > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength);<br>
> > > > > > mode_.maxShutter = Duration::max();<br>
> <br>
> -- <br>
> Regards,<br>
> <br>
> Laurent Pinchart<br>
</blockquote></div></div>