| Message ID | 20240426111815.10763-1-naush@raspberrypi.com |
|---|---|
| State | New |
| 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();
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(+)