| Message ID | 20240426111815.10763-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 299e5278bd7657c785b7adac0c4f02d7e42f22c2 |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the patch. On Fri, 26 Apr 2024 at 12:18, Naushir Patuck <naush@raspberrypi.com> wrote: > > The maximum shutter speed calculation in the cam-helper relied on > the frame duration limits being correctly set in the cam-helper's mode > structure. This was not the case on first startup, so the maximum > shutter speed reported back via the ControlInfo was incorrect. > > Fix this by setting up the camera mode in the cam-helper before querying > for the max shutter value. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Yes, looks sensible to me. Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > --- > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index 149a133ab662..1d12262bda01 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > > + /* > + * We need to give the helper the min/max frame durations so it can calculate > + * the correct exposure limits below. > + */ > + helper_->setCameraMode(mode_); > + > /* Shutter speed is calculated based on the limits of the frame durations. */ > mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > mode_.maxShutter = Duration::max(); > -- > 2.34.1 >
Hi Naush, Thank you for the patch. On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > The maximum shutter speed calculation in the cam-helper relied on > the frame duration limits being correctly set in the cam-helper's mode > structure. This was not the case on first startup, so the maximum > shutter speed reported back via the ControlInfo was incorrect. > > Fix this by setting up the camera mode in the cam-helper before querying > for the max shutter value. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index 149a133ab662..1d12262bda01 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > > + /* > + * We need to give the helper the min/max frame durations so it can calculate > + * the correct exposure limits below. > + */ > + helper_->setCameraMode(mode_); > + Don't you end up doing this twice, once here, and once in IpaBase::configure(), which calls IpaBase::setMode() ? > /* Shutter speed is calculated based on the limits of the frame durations. */ > mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > mode_.maxShutter = Duration::max();
Hi Laurent, On Wed, 1 May 2024 at 16:53, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > The maximum shutter speed calculation in the cam-helper relied on > > the frame duration limits being correctly set in the cam-helper's mode > > structure. This was not the case on first startup, so the maximum > > shutter speed reported back via the ControlInfo was incorrect. > > > > Fix this by setting up the camera mode in the cam-helper before querying > > for the max shutter value. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp > b/src/ipa/rpi/common/ipa_base.cpp > > index 149a133ab662..1d12262bda01 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo > &sensorInfo) > > mode_.minAnalogueGain = > helper_->gain(gainCtrl.min().get<int32_t>()); > > mode_.maxAnalogueGain = > helper_->gain(gainCtrl.max().get<int32_t>()); > > > > + /* > > + * We need to give the helper the min/max frame durations so it > can calculate > > + * the correct exposure limits below. > > + */ > > + helper_->setCameraMode(mode_); > > + > > Don't you end up doing this twice, once here, and once in > IpaBase::configure(), which calls IpaBase::setMode() ? > Yes, it does happen twice. The other option would be to pass a const reference to mode_ into the helper through setMode(), allowing us only do it once. But that was a bit more involved over this more trivial fix. I can change to do that if folks prefer. Naush > > /* Shutter speed is calculated based on the limits of the frame > durations. */ > > mode_.minShutter = > helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > mode_.maxShutter = Duration::max(); > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote: > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote: > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > > The maximum shutter speed calculation in the cam-helper relied on > > > the frame duration limits being correctly set in the cam-helper's mode > > > structure. This was not the case on first startup, so the maximum > > > shutter speed reported back via the ControlInfo was incorrect. > > > > > > Fix this by setting up the camera mode in the cam-helper before querying > > > for the max shutter value. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > index 149a133ab662..1d12262bda01 100644 > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > > > > > > + /* > > > + * We need to give the helper the min/max frame durations so it can calculate > > > + * the correct exposure limits below. > > > + */ > > > + helper_->setCameraMode(mode_); > > > + > > > > Don't you end up doing this twice, once here, and once in > > IpaBase::configure(), which calls IpaBase::setMode() ? > > Yes, it does happen twice. The other option would be to pass a const > reference to mode_ into the helper through setMode(), allowing us only do > it once. I don't think I follow you. > But that was a bit more involved over this more trivial fix. I > can change to do that if folks prefer. > > > > /* Shutter speed is calculated based on the limits of the frame durations. */ > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > > mode_.maxShutter = Duration::max();
Hi Laurent, On Sat, 4 May 2024 at 16:04, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote: > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote: > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > > > The maximum shutter speed calculation in the cam-helper relied on > > > > the frame duration limits being correctly set in the cam-helper's mode > > > > structure. This was not the case on first startup, so the maximum > > > > shutter speed reported back via the ControlInfo was incorrect. > > > > > > > > Fix this by setting up the camera mode in the cam-helper before querying > > > > for the max shutter value. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > index 149a133ab662..1d12262bda01 100644 > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > > > > > > > > + /* > > > > + * We need to give the helper the min/max frame durations so it can calculate > > > > + * the correct exposure limits below. > > > > + */ > > > > + helper_->setCameraMode(mode_); > > > > + > > > > > > Don't you end up doing this twice, once here, and once in > > > IpaBase::configure(), which calls IpaBase::setMode() ? > > > > Yes, it does happen twice. The other option would be to pass a const > > reference to mode_ into the helper through setMode(), allowing us only do > > it once. > > I don't think I follow you. What I mean is that we don't give the cam helper its own copy of the mode structure, but pass it a const reference to the IPA's mode structure. This would solve the problem with the calculation here. Regards, Naush > > > But that was a bit more involved over this more trivial fix. I > > can change to do that if folks prefer. > > > > > > /* Shutter speed is calculated based on the limits of the frame durations. */ > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > > > mode_.maxShutter = Duration::max(); > > -- > Regards, > > Laurent Pinchart
Hi Naush, On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote: > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote: > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote: > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote: > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > > > > The maximum shutter speed calculation in the cam-helper relied on > > > > > the frame duration limits being correctly set in the cam-helper's mode > > > > > structure. This was not the case on first startup, so the maximum > > > > > shutter speed reported back via the ControlInfo was incorrect. > > > > > > > > > > Fix this by setting up the camera mode in the cam-helper before querying > > > > > for the max shutter value. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > --- > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > > index 149a133ab662..1d12262bda01 100644 > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > > > > > > > > > > + /* > > > > > + * We need to give the helper the min/max frame durations so it can calculate > > > > > + * the correct exposure limits below. > > > > > + */ > > > > > + helper_->setCameraMode(mode_); > > > > > + > > > > > > > > Don't you end up doing this twice, once here, and once in > > > > IpaBase::configure(), which calls IpaBase::setMode() ? > > > > > > Yes, it does happen twice. The other option would be to pass a const > > > reference to mode_ into the helper through setMode(), allowing us only do > > > it once. > > > > I don't think I follow you. > > What I mean is that we don't give the cam helper its own copy of the > mode structure, but pass it a const reference to the IPA's mode > structure. This would solve the problem with the calculation here. It's probably me, but I still don't get it :-) We have void CamHelper::setCameraMode(const CameraMode &mode) { mode_ = mode; if (parser_) { parser_->reset(); parser_->setBitsPerPixel(mode.bitdepth); parser_->setLineLengthBytes(0); /* We use SetBufferSize. */ } } called in IpaBase::configure(). This patch adds another call in IpaBase::setMode(). You wrote > The other option would be to pass a const reference to mode_ into the > helper through setMode() Doesn't this patch does exactly that ? How is it another option ? /me is confused > > > But that was a bit more involved over this more trivial fix. I > > > can change to do that if folks prefer. > > > > > > > > /* Shutter speed is calculated based on the limits of the frame durations. */ > > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > > > > mode_.maxShutter = Duration::max();
Quoting Naushir Patuck (2024-05-07 11:32:54) > Hi Laurent, > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Naush, > > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote: > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote: > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > > > > The maximum shutter speed calculation in the cam-helper relied on > > > > > the frame duration limits being correctly set in the cam-helper's mode > > > > > structure. This was not the case on first startup, so the maximum > > > > > shutter speed reported back via the ControlInfo was incorrect. > > > > > > > > > > Fix this by setting up the camera mode in the cam-helper before querying > > > > > for the max shutter value. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > --- > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > > index 149a133ab662..1d12262bda01 100644 > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > > > > > > > > > > + /* > > > > > + * We need to give the helper the min/max frame durations so it can calculate > > > > > + * the correct exposure limits below. > > > > > + */ > > > > > + helper_->setCameraMode(mode_); > > > > > + > > > > > > > > Don't you end up doing this twice, once here, and once in > > > > IpaBase::configure(), which calls IpaBase::setMode() ? > > > > > > Yes, it does happen twice. The other option would be to pass a const > > > reference to mode_ into the helper through setMode(), allowing us only do > > > it once. > > > > I don't think I follow you. > > What I mean is that we don't give the cam helper its own copy of the > mode structure, but pass it a const reference to the IPA's mode > structure. This would solve the problem with the calculation here. > > Regards, > Naush > > > > > > But that was a bit more involved over this more trivial fix. I > > > can change to do that if folks prefer. This is your IPA. I'm happy to merge this if you believe it's ready as you want it. Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> -- Kieran > > > > > > > > /* Shutter speed is calculated based on the limits of the frame durations. */ > > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > > > > mode_.maxShutter = Duration::max(); > > > > -- > > Regards, > > > > Laurent Pinchart
On Thu, May 09, 2024 at 11:39:35AM +0100, Kieran Bingham wrote: > Quoting Naushir Patuck (2024-05-07 11:32:54) > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote: > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote: > > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote: > > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > > > > > The maximum shutter speed calculation in the cam-helper relied on > > > > > > the frame duration limits being correctly set in the cam-helper's mode > > > > > > structure. This was not the case on first startup, so the maximum > > > > > > shutter speed reported back via the ControlInfo was incorrect. > > > > > > > > > > > > Fix this by setting up the camera mode in the cam-helper before querying > > > > > > for the max shutter value. > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > --- > > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > > > index 149a133ab662..1d12262bda01 100644 > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > > > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > > > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > > > > > > > > > > > > + /* > > > > > > + * We need to give the helper the min/max frame durations so it can calculate > > > > > > + * the correct exposure limits below. > > > > > > + */ > > > > > > + helper_->setCameraMode(mode_); > > > > > > + > > > > > > > > > > Don't you end up doing this twice, once here, and once in > > > > > IpaBase::configure(), which calls IpaBase::setMode() ? > > > > > > > > Yes, it does happen twice. The other option would be to pass a const > > > > reference to mode_ into the helper through setMode(), allowing us only do > > > > it once. > > > > > > I don't think I follow you. > > > > What I mean is that we don't give the cam helper its own copy of the > > mode structure, but pass it a const reference to the IPA's mode > > structure. This would solve the problem with the calculation here. > > > > > > But that was a bit more involved over this more trivial fix. I > > > > can change to do that if folks prefer. > > This is your IPA. I'm happy to merge this if you believe it's ready as > you want it. I forgot to clearly mention in my previous reply that I'm OK with that too. It would be nice to avoid the duplicated call and that would be my preference, but I won't block this patch just for this reason. > Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > /* Shutter speed is calculated based on the limits of the frame durations. */ > > > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > > > > > mode_.maxShutter = Duration::max();
On Thu, 9 May 2024 at 11:23, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote: > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote: > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote: > > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote: > > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > > > > > The maximum shutter speed calculation in the cam-helper relied on > > > > > > the frame duration limits being correctly set in the > cam-helper's mode > > > > > > structure. This was not the case on first startup, so the maximum > > > > > > shutter speed reported back via the ControlInfo was incorrect. > > > > > > > > > > > > Fix this by setting up the camera mode in the cam-helper before > querying > > > > > > for the max shutter value. > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > --- > > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp > b/src/ipa/rpi/common/ipa_base.cpp > > > > > > index 149a133ab662..1d12262bda01 100644 > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const > IPACameraSensorInfo &sensorInfo) > > > > > > mode_.minAnalogueGain = > helper_->gain(gainCtrl.min().get<int32_t>()); > > > > > > mode_.maxAnalogueGain = > helper_->gain(gainCtrl.max().get<int32_t>()); > > > > > > > > > > > > + /* > > > > > > + * We need to give the helper the min/max frame durations > so it can calculate > > > > > > + * the correct exposure limits below. > > > > > > + */ > > > > > > + helper_->setCameraMode(mode_); > > > > > > + > > > > > > > > > > Don't you end up doing this twice, once here, and once in > > > > > IpaBase::configure(), which calls IpaBase::setMode() ? > > > > > > > > Yes, it does happen twice. The other option would be to pass a const > > > > reference to mode_ into the helper through setMode(), allowing us > only do > > > > it once. > > > > > > I don't think I follow you. > > > > What I mean is that we don't give the cam helper its own copy of the > > mode structure, but pass it a const reference to the IPA's mode > > structure. This would solve the problem with the calculation here. > > It's probably me, but I still don't get it :-) We have > > void CamHelper::setCameraMode(const CameraMode &mode) > { > mode_ = mode; > if (parser_) { > parser_->reset(); > parser_->setBitsPerPixel(mode.bitdepth); > parser_->setLineLengthBytes(0); /* We use SetBufferSize. */ > } > } > > called in IpaBase::configure(). This patch adds another call in > IpaBase::setMode(). You wrote > > > The other option would be to pass a const reference to mode_ into the > > helper through setMode() > > Doesn't this patch does exactly that ? How is it another option ? > > /me is confused In CamHelper::setCameraMode(const CameraMode &mode), I would have to store the const reference rather than make a copy of CameraMode mode: diff --git a/src/ipa/rpi/cam_helper/cam_helper.h b/src/ipa/rpi/cam_helper/cam_helper.h index 58a4b202d5a8..eb2a56484606 100644 --- a/src/ipa/rpi/cam_helper/cam_helper.h +++ b/src/ipa/rpi/cam_helper/cam_helper.h @@ -107,7 +107,7 @@ protected: Metadata &metadata) const; std::unique_ptr<MdParser> parser_; - CameraMode mode_; + const CameraMode &mode_; Then in the IPA if I call CamHelper::setCameraMode() before IpaBase::setMode() + a bit of other refactoring, I think I can achieve what I want without a double call.. The "other bit of refactoring" is the bit I'm a bit hesitant to work on for such a simple fix :) Regards, Naush > > > > > But that was a bit more involved over this more trivial fix. I > > > > can change to do that if folks prefer. > > > > > > > > > > /* Shutter speed is calculated based on the limits of the > frame durations. */ > > > > > > mode_.minShutter = > helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > > > > > mode_.maxShutter = Duration::max(); > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Thu, May 09, 2024 at 11:51:05AM +0100, Naushir Patuck wrote: > On Thu, 9 May 2024 at 11:23, Laurent Pinchart wrote: > > On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote: > > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote: > > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote: > > > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote: > > > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > > > > > > The maximum shutter speed calculation in the cam-helper relied on > > > > > > > the frame duration limits being correctly set in the cam-helper's mode > > > > > > > structure. This was not the case on first startup, so the maximum > > > > > > > shutter speed reported back via the ControlInfo was incorrect. > > > > > > > > > > > > > > Fix this by setting up the camera mode in the cam-helper before querying > > > > > > > for the max shutter value. > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > --- > > > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > > > > index 149a133ab662..1d12262bda01 100644 > > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > > > > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > > > > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > > > > > > > > > > > > > > + /* > > > > > > > + * We need to give the helper the min/max frame durations so it can calculate > > > > > > > + * the correct exposure limits below. > > > > > > > + */ > > > > > > > + helper_->setCameraMode(mode_); > > > > > > > + > > > > > > > > > > > > Don't you end up doing this twice, once here, and once in > > > > > > IpaBase::configure(), which calls IpaBase::setMode() ? > > > > > > > > > > Yes, it does happen twice. The other option would be to pass a const > > > > > reference to mode_ into the helper through setMode(), allowing us only do > > > > > it once. > > > > > > > > I don't think I follow you. > > > > > > What I mean is that we don't give the cam helper its own copy of the > > > mode structure, but pass it a const reference to the IPA's mode > > > structure. This would solve the problem with the calculation here. > > > > It's probably me, but I still don't get it :-) We have > > > > void CamHelper::setCameraMode(const CameraMode &mode) > > { > > mode_ = mode; > > if (parser_) { > > parser_->reset(); > > parser_->setBitsPerPixel(mode.bitdepth); > > parser_->setLineLengthBytes(0); /* We use SetBufferSize. */ > > } > > } > > > > called in IpaBase::configure(). This patch adds another call in > > IpaBase::setMode(). You wrote > > > > > The other option would be to pass a const reference to mode_ into the > > > helper through setMode() > > > > Doesn't this patch does exactly that ? How is it another option ? > > > > /me is confused > > In CamHelper::setCameraMode(const CameraMode &mode), I would have to store > the const reference rather than make a copy of CameraMode mode: > > diff --git a/src/ipa/rpi/cam_helper/cam_helper.h > b/src/ipa/rpi/cam_helper/cam_helper.h > index 58a4b202d5a8..eb2a56484606 100644 > --- a/src/ipa/rpi/cam_helper/cam_helper.h > +++ b/src/ipa/rpi/cam_helper/cam_helper.h > @@ -107,7 +107,7 @@ protected: > Metadata &metadata) const; > > std::unique_ptr<MdParser> parser_; > - CameraMode mode_; > + const CameraMode &mode_; > > Then in the IPA if I call CamHelper::setCameraMode() > before IpaBase::setMode() + a bit of other refactoring, I think I can > achieve what I want without a double call.. The "other bit of refactoring" > is the bit I'm a bit hesitant to work on for such a simple fix :) Aahhh OK, I get it now. The above won't compile though, you can't reassign a reference, it can only be initialized at object construction time. You could use a pointer, but I think that will make the code more difficult to understand, with changes being implicitly shared between components. Let's stick with this patch for the time being. There's probably a chance to refactor the code to make the interface between the components a bit cleaner, but that's a gut feeling at this point. Or maybe a wishful thinking that there is always a way to make pieces fall neatly into place :-) > > > > > But that was a bit more involved over this more trivial fix. I > > > > > can change to do that if folks prefer. > > > > > > > > > > > > /* Shutter speed is calculated based on the limits of the frame durations. */ > > > > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > > > > > > mode_.maxShutter = Duration::max();
Quoting Laurent Pinchart (2024-05-09 11:23:14) > Hi Naush, > > On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote: > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote: > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote: > > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote: > > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > > > > > The maximum shutter speed calculation in the cam-helper relied on > > > > > > the frame duration limits being correctly set in the cam-helper's mode > > > > > > structure. This was not the case on first startup, so the maximum > > > > > > shutter speed reported back via the ControlInfo was incorrect. > > > > > > > > > > > > Fix this by setting up the camera mode in the cam-helper before querying > > > > > > for the max shutter value. > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > --- > > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > > > index 149a133ab662..1d12262bda01 100644 > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > > > > > > mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); > > > > > > mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); > > > > > > > > > > > > + /* > > > > > > + * We need to give the helper the min/max frame durations so it can calculate > > > > > > + * the correct exposure limits below. > > > > > > + */ > > > > > > + helper_->setCameraMode(mode_); > > > > > > + > > > > > > > > > > Don't you end up doing this twice, once here, and once in > > > > > IpaBase::configure(), which calls IpaBase::setMode() ? > > > > > > > > Yes, it does happen twice. The other option would be to pass a const > > > > reference to mode_ into the helper through setMode(), allowing us only do > > > > it once. > > > > > > I don't think I follow you. > > > > What I mean is that we don't give the cam helper its own copy of the > > mode structure, but pass it a const reference to the IPA's mode > > structure. This would solve the problem with the calculation here. > > It's probably me, but I still don't get it :-) We have > > void CamHelper::setCameraMode(const CameraMode &mode) > { > mode_ = mode; > if (parser_) { > parser_->reset(); > parser_->setBitsPerPixel(mode.bitdepth); > parser_->setLineLengthBytes(0); /* We use SetBufferSize. */ > } > } > > called in IpaBase::configure(). This patch adds another call in > IpaBase::setMode(). You wrote > > > The other option would be to pass a const reference to mode_ into the > > helper through setMode() > > Doesn't this patch does exactly that ? How is it another option ? > > /me is confused I can see the duplication. I guess the question is ... Naush - do you want to remove the second call? I don't see any need to change the constness of it currently? The call flow I see is: IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms, ConfigResult *result) { ... /* Re-assemble camera mode using the sensor info. */ setMode(sensorInfo); -> IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) { /* KB: Which with this patch calls */ ... mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); + /* + * We need to give the helper the min/max frame durations so it can calculate + * the correct exposure limits below. + */ + helper_->setCameraMode(mode_); /* Shutter speed is calculated based on the limits of the frame durations. */ mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); mode_.maxShutter = Duration::max(); helper_->getBlanking(mode_.maxShutter, mode_.minFrameDuration, mode_.maxFrameDuration); ... } /* KB: And then flows directly into the next two lines to call * helper_->setCameraMode(mode_); again */ mode_.transform = static_cast<libcamera::Transform>(params.transform); - - /* Pass the camera mode to the CamHelper to setup algorithms. */ - helper_->setCameraMode(mode_); ... } So I think the open question is simply, should this patch remove the lines I've prefixed with '-' as it has added the lines prefixed with '+'.... I think doing so require setting the mode_.transform before calling setMode(sensorInfo), but that looks like it's all it would do? Naush - if you're happy with the patch as is, I can merge it already. There's probably some other interactions we've missed that you have better visibility on. -- Kieran > > > > > But that was a bit more involved over this more trivial fix. I > > > > can change to do that if folks prefer. > > > > > > > > > > /* Shutter speed is calculated based on the limits of the frame durations. */ > > > > > > mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > > > > > mode_.maxShutter = Duration::max(); > > -- > Regards, > > Laurent Pinchart
On Thu, 9 May 2024 at 12:02, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Quoting Laurent Pinchart (2024-05-09 11:23:14) > > Hi Naush, > > > > On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote: > > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote: > > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote: > > > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote: > > > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > > > > > > The maximum shutter speed calculation in the cam-helper relied > on > > > > > > > the frame duration limits being correctly set in the > cam-helper's mode > > > > > > > structure. This was not the case on first startup, so the > maximum > > > > > > > shutter speed reported back via the ControlInfo was incorrect. > > > > > > > > > > > > > > Fix this by setting up the camera mode in the cam-helper > before querying > > > > > > > for the max shutter value. > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > --- > > > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp > b/src/ipa/rpi/common/ipa_base.cpp > > > > > > > index 149a133ab662..1d12262bda01 100644 > > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const > IPACameraSensorInfo &sensorInfo) > > > > > > > mode_.minAnalogueGain = > helper_->gain(gainCtrl.min().get<int32_t>()); > > > > > > > mode_.maxAnalogueGain = > helper_->gain(gainCtrl.max().get<int32_t>()); > > > > > > > > > > > > > > + /* > > > > > > > + * We need to give the helper the min/max frame > durations so it can calculate > > > > > > > + * the correct exposure limits below. > > > > > > > + */ > > > > > > > + helper_->setCameraMode(mode_); > > > > > > > + > > > > > > > > > > > > Don't you end up doing this twice, once here, and once in > > > > > > IpaBase::configure(), which calls IpaBase::setMode() ? > > > > > > > > > > Yes, it does happen twice. The other option would be to pass a > const > > > > > reference to mode_ into the helper through setMode(), allowing us > only do > > > > > it once. > > > > > > > > I don't think I follow you. > > > > > > What I mean is that we don't give the cam helper its own copy of the > > > mode structure, but pass it a const reference to the IPA's mode > > > structure. This would solve the problem with the calculation here. > > > > It's probably me, but I still don't get it :-) We have > > > > void CamHelper::setCameraMode(const CameraMode &mode) > > { > > mode_ = mode; > > if (parser_) { > > parser_->reset(); > > parser_->setBitsPerPixel(mode.bitdepth); > > parser_->setLineLengthBytes(0); /* We use SetBufferSize. > */ > > } > > } > > > > called in IpaBase::configure(). This patch adds another call in > > IpaBase::setMode(). You wrote > > > > > The other option would be to pass a const reference to mode_ into the > > > helper through setMode() > > > > Doesn't this patch does exactly that ? How is it another option ? > > > > /me is confused > > > I can see the duplication. > > I guess the question is ... Naush - do you want to remove the second > call? I don't see any need to change the constness of it currently? > > > The call flow I see is: > > IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const > ConfigParams ¶ms, > ConfigResult *result) > { > ... > /* Re-assemble camera mode using the sensor info. */ > setMode(sensorInfo); > -> > IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > { /* KB: Which with this patch calls */ > ... > mode_.minAnalogueGain = > helper_->gain(gainCtrl.min().get<int32_t>()); > mode_.maxAnalogueGain = > helper_->gain(gainCtrl.max().get<int32_t>()); > > + /* > + * We need to give the helper the min/max frame durations > so it can calculate > + * the correct exposure limits below. > + */ > + helper_->setCameraMode(mode_); > > /* Shutter speed is calculated based on the limits of the > frame durations. */ > mode_.minShutter = > helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > mode_.maxShutter = Duration::max(); > helper_->getBlanking(mode_.maxShutter, > mode_.minFrameDuration, > mode_.maxFrameDuration); > ... > } > > /* KB: And then flows directly into the next two lines to call > * helper_->setCameraMode(mode_); again */ > > mode_.transform = > static_cast<libcamera::Transform>(params.transform); > - > - /* Pass the camera mode to the CamHelper to setup algorithms. */ > - helper_->setCameraMode(mode_); > > ... > } > > So I think the open question is simply, should this patch remove the > lines I've prefixed with '-' as it has added the lines prefixed with > '+'.... > > I think doing so require setting the mode_.transform before calling > setMode(sensorInfo), but that looks like it's all it would do? > Almost but not quite. The copy in the helper will not have the updated min/max shutter. > > > Naush - if you're happy with the patch as is, I can merge it already. > There's probably some other interactions we've missed that you have > better visibility on. > I would suggest sticking with the current patch. It can get fiddly quite quickly moving these bits of logic around. Regards, Naush > > -- > Kieran > > > > > > > > > > But that was a bit more involved over this more trivial fix. I > > > > > can change to do that if folks prefer. > > > > > > > > > > > > /* Shutter speed is calculated based on the limits of > the frame durations. */ > > > > > > > mode_.minShutter = > helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > > > > > > mode_.maxShutter = Duration::max(); > > > > -- > > Regards, > > > > Laurent Pinchart >
Quoting Naushir Patuck (2024-05-09 12:43:03) > On Thu, 9 May 2024 at 12:02, Kieran Bingham <kieran.bingham@ideasonboard.com> > wrote: > > > Quoting Laurent Pinchart (2024-05-09 11:23:14) > > > Hi Naush, > > > > > > On Tue, May 07, 2024 at 11:32:54AM +0100, Naushir Patuck wrote: > > > > On Sat, 4 May 2024 at 16:04, Laurent Pinchart wrote: > > > > > On Wed, May 01, 2024 at 05:04:03PM +0100, Naushir Patuck wrote: > > > > > > On Wed, 1 May 2024 at 16:53, Laurent Pinchart wrote: > > > > > > > On Fri, Apr 26, 2024 at 12:18:15PM +0100, Naushir Patuck wrote: > > > > > > > > The maximum shutter speed calculation in the cam-helper relied > > on > > > > > > > > the frame duration limits being correctly set in the > > cam-helper's mode > > > > > > > > structure. This was not the case on first startup, so the > > maximum > > > > > > > > shutter speed reported back via the ControlInfo was incorrect. > > > > > > > > > > > > > > > > Fix this by setting up the camera mode in the cam-helper > > before querying > > > > > > > > for the max shutter value. > > > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > > --- > > > > > > > > src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ > > > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp > > b/src/ipa/rpi/common/ipa_base.cpp > > > > > > > > index 149a133ab662..1d12262bda01 100644 > > > > > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > > > > > @@ -592,6 +592,12 @@ void IpaBase::setMode(const > > IPACameraSensorInfo &sensorInfo) > > > > > > > > mode_.minAnalogueGain = > > helper_->gain(gainCtrl.min().get<int32_t>()); > > > > > > > > mode_.maxAnalogueGain = > > helper_->gain(gainCtrl.max().get<int32_t>()); > > > > > > > > > > > > > > > > + /* > > > > > > > > + * We need to give the helper the min/max frame > > durations so it can calculate > > > > > > > > + * the correct exposure limits below. > > > > > > > > + */ > > > > > > > > + helper_->setCameraMode(mode_); > > > > > > > > + > > > > > > > > > > > > > > Don't you end up doing this twice, once here, and once in > > > > > > > IpaBase::configure(), which calls IpaBase::setMode() ? > > > > > > > > > > > > Yes, it does happen twice. The other option would be to pass a > > const > > > > > > reference to mode_ into the helper through setMode(), allowing us > > only do > > > > > > it once. > > > > > > > > > > I don't think I follow you. > > > > > > > > What I mean is that we don't give the cam helper its own copy of the > > > > mode structure, but pass it a const reference to the IPA's mode > > > > structure. This would solve the problem with the calculation here. > > > > > > It's probably me, but I still don't get it :-) We have > > > > > > void CamHelper::setCameraMode(const CameraMode &mode) > > > { > > > mode_ = mode; > > > if (parser_) { > > > parser_->reset(); > > > parser_->setBitsPerPixel(mode.bitdepth); > > > parser_->setLineLengthBytes(0); /* We use SetBufferSize. > > */ > > > } > > > } > > > > > > called in IpaBase::configure(). This patch adds another call in > > > IpaBase::setMode(). You wrote > > > > > > > The other option would be to pass a const reference to mode_ into the > > > > helper through setMode() > > > > > > Doesn't this patch does exactly that ? How is it another option ? > > > > > > /me is confused > > > > > > I can see the duplication. > > > > I guess the question is ... Naush - do you want to remove the second > > call? I don't see any need to change the constness of it currently? > > > > > > The call flow I see is: > > > > IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const > > ConfigParams ¶ms, > > ConfigResult *result) > > { > > ... > > /* Re-assemble camera mode using the sensor info. */ > > setMode(sensorInfo); > > -> > > IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) > > { /* KB: Which with this patch calls */ > > ... > > mode_.minAnalogueGain = > > helper_->gain(gainCtrl.min().get<int32_t>()); > > mode_.maxAnalogueGain = > > helper_->gain(gainCtrl.max().get<int32_t>()); > > > > + /* > > + * We need to give the helper the min/max frame durations > > so it can calculate > > + * the correct exposure limits below. > > + */ > > + helper_->setCameraMode(mode_); > > > > /* Shutter speed is calculated based on the limits of the > > frame durations. */ > > mode_.minShutter = > > helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > mode_.maxShutter = Duration::max(); > > helper_->getBlanking(mode_.maxShutter, > > mode_.minFrameDuration, > > mode_.maxFrameDuration); > > ... > > } > > > > /* KB: And then flows directly into the next two lines to call > > * helper_->setCameraMode(mode_); again */ > > > > mode_.transform = > > static_cast<libcamera::Transform>(params.transform); > > - > > - /* Pass the camera mode to the CamHelper to setup algorithms. */ > > - helper_->setCameraMode(mode_); > > > > ... > > } > > > > So I think the open question is simply, should this patch remove the > > lines I've prefixed with '-' as it has added the lines prefixed with > > '+'.... > > > > I think doing so require setting the mode_.transform before calling > > setMode(sensorInfo), but that looks like it's all it would do? > > > > Almost but not quite. The copy in the helper will not have the updated > min/max shutter. > > > > > > > > Naush - if you're happy with the patch as is, I can merge it already. > > There's probably some other interactions we've missed that you have > > better visibility on. > > > > I would suggest sticking with the current patch. It can get fiddly quite > quickly moving these bits of logic around. Sold, merging. -- Kieran > > Regards, > Naush > > > > > > -- > > Kieran > > > > > > > > > > > > > > > But that was a bit more involved over this more trivial fix. I > > > > > > can change to do that if folks prefer. > > > > > > > > > > > > > > /* Shutter speed is calculated based on the limits of > > the frame durations. */ > > > > > > > > mode_.minShutter = > > helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); > > > > > > > > mode_.maxShutter = Duration::max(); > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart > >
diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index 149a133ab662..1d12262bda01 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -592,6 +592,12 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo) mode_.minAnalogueGain = helper_->gain(gainCtrl.min().get<int32_t>()); mode_.maxAnalogueGain = helper_->gain(gainCtrl.max().get<int32_t>()); + /* + * We need to give the helper the min/max frame durations so it can calculate + * the correct exposure limits below. + */ + helper_->setCameraMode(mode_); + /* Shutter speed is calculated based on the limits of the frame durations. */ mode_.minShutter = helper_->exposure(shutterCtrl.min().get<int32_t>(), mode_.minLineLength); mode_.maxShutter = Duration::max();
The maximum shutter speed calculation in the cam-helper relied on the frame duration limits being correctly set in the cam-helper's mode structure. This was not the case on first startup, so the maximum shutter speed reported back via the ControlInfo was incorrect. Fix this by setting up the camera mode in the cam-helper before querying for the max shutter value. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/rpi/common/ipa_base.cpp | 6 ++++++ 1 file changed, 6 insertions(+)