| Message ID | 20220124103107.1799464-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | e875503bc4a40e5043fb045942535b865692ff60 |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the patch! On Mon, 24 Jan 2022 at 10:31, Naushir Patuck <naush@raspberrypi.com> wrote: > > Limit the gain code to the maximum value reported by the sensor controls when > sending to DelayedControls. The AGC algorithm will handle a lower gain used by > the sensor, provided provided it knows the actual gain used. This change ensures > that DelayedControls will never report an unclipped gain used. Apart from s/provided provided/provided/: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 0ed41385018a..a72d516f84ee 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -173,6 +173,9 @@ private: > /* Frame duration (1/fps) limits. */ > Duration minFrameDuration_; > Duration maxFrameDuration_; > + > + /* Maximum gain code for the sensor. */ > + uint32_t maxSensorGainCode_; > }; > > int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) > @@ -357,6 +360,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, > return -1; > } > > + maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > + > /* Setup a metadata ControlList to output metadata. */ > libcameraMetadata_ = ControlList(controls::controls); > > @@ -1113,6 +1118,13 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) > { > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); > > + /* > + * Ensure anything larger than the max gain code will not be passed to > + * DelayedControls. The AGC will correctly handle a lower gain returned > + * by the sensor, provided it knows the actual gain used. > + */ > + gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); > + > /* GetVBlanking might clip exposure time to the fps limits. */ > Duration exposure = agcStatus->shutter_time; > int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_); > -- > 2.25.1 >
Hi Naush, Quoting Naushir Patuck (2022-01-24 10:31:06) > Limit the gain code to the maximum value reported by the sensor controls when > sending to DelayedControls. The AGC algorithm will handle a lower gain used by > the sensor, provided provided it knows the actual gain used. This change ensures > that DelayedControls will never report an unclipped gain used. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 0ed41385018a..a72d516f84ee 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -173,6 +173,9 @@ private: > /* Frame duration (1/fps) limits. */ > Duration minFrameDuration_; > Duration maxFrameDuration_; > + > + /* Maximum gain code for the sensor. */ > + uint32_t maxSensorGainCode_; Any risk of needing to clamp the minimum value too? > }; > > int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) > @@ -357,6 +360,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, > return -1; > } > > + maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > + > /* Setup a metadata ControlList to output metadata. */ > libcameraMetadata_ = ControlList(controls::controls); > > @@ -1113,6 +1118,13 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) > { > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); > > + /* > + * Ensure anything larger than the max gain code will not be passed to > + * DelayedControls. The AGC will correctly handle a lower gain returned > + * by the sensor, provided it knows the actual gain used. > + */ > + gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); > + Sounds ok to me though. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > /* GetVBlanking might clip exposure time to the fps limits. */ > Duration exposure = agcStatus->shutter_time; > int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_); > -- > 2.25.1 >
Hi Kieran, Thanks for the review (this + all the other patches as well)! On Tue, 1 Feb 2022 at 00:00, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > Quoting Naushir Patuck (2022-01-24 10:31:06) > > Limit the gain code to the maximum value reported by the sensor controls > when > > sending to DelayedControls. The AGC algorithm will handle a lower gain > used by > > the sensor, provided provided it knows the actual gain used. This change > ensures > > that DelayedControls will never report an unclipped gain used. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index 0ed41385018a..a72d516f84ee 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -173,6 +173,9 @@ private: > > /* Frame duration (1/fps) limits. */ > > Duration minFrameDuration_; > > Duration maxFrameDuration_; > > + > > + /* Maximum gain code for the sensor. */ > > + uint32_t maxSensorGainCode_; > > Any risk of needing to clamp the minimum value too? > This is a good point. I made the assumption that a minimum gain of 1.0 is universal, but perhaps not... David, what do you think? I recall you had played with devices where this may possible not be true? Regards, Naush > > > }; > > > > int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig > *sensorConfig) > > @@ -357,6 +360,8 @@ int IPARPi::configure(const IPACameraSensorInfo > &sensorInfo, > > return -1; > > } > > > > + maxSensorGainCode_ = > sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); > > + > > /* Setup a metadata ControlList to output metadata. */ > > libcameraMetadata_ = ControlList(controls::controls); > > > > @@ -1113,6 +1118,13 @@ void IPARPi::applyAGC(const struct AgcStatus > *agcStatus, ControlList &ctrls) > > { > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); > > > > + /* > > + * Ensure anything larger than the max gain code will not be > passed to > > + * DelayedControls. The AGC will correctly handle a lower gain > returned > > + * by the sensor, provided it knows the actual gain used. > > + */ > > + gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); > > + > > Sounds ok to me though. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > /* GetVBlanking might clip exposure time to the fps limits. */ > > Duration exposure = agcStatus->shutter_time; > > int32_t vblanking = helper_->GetVBlanking(exposure, > minFrameDuration_, maxFrameDuration_); > > -- > > 2.25.1 > > >
Hi everyone On Tue, 1 Feb 2022 at 08:48, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi Kieran, > > Thanks for the review (this + all the other patches as well)! > > On Tue, 1 Feb 2022 at 00:00, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: >> >> Hi Naush, >> >> Quoting Naushir Patuck (2022-01-24 10:31:06) >> > Limit the gain code to the maximum value reported by the sensor controls when >> > sending to DelayedControls. The AGC algorithm will handle a lower gain used by >> > the sensor, provided provided it knows the actual gain used. This change ensures >> > that DelayedControls will never report an unclipped gain used. >> > >> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >> > --- >> > src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ >> > 1 file changed, 12 insertions(+) >> > >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp >> > index 0ed41385018a..a72d516f84ee 100644 >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp >> > @@ -173,6 +173,9 @@ private: >> > /* Frame duration (1/fps) limits. */ >> > Duration minFrameDuration_; >> > Duration maxFrameDuration_; >> > + >> > + /* Maximum gain code for the sensor. */ >> > + uint32_t maxSensorGainCode_; >> >> Any risk of needing to clamp the minimum value too? > > > This is a good point. > I made the assumption that a minimum gain of 1.0 is universal, but perhaps not... > David, what do you think? I recall you had played with devices where this may possible > not be true? > It's a fair question, but I'm not inclined to clamp the minimum as well. I think handling that would require extra code in the AGC (I have other things to tackle right now...) or maybe editing exposure profiles on a per-sensor basis (also undesirable,). Actually I have one sensor which has a minimum gain of 1.12 for *some*, but not all, readout modes, but I take care of all that in the cam helper. So anyway, I'd rather fix the problem that we really have and leave this question on the back burner for now. We could re-consider it if it ever becomes a real thing. David > Regards, > Naush > >> >> >> > }; >> > >> > int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) >> > @@ -357,6 +360,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, >> > return -1; >> > } >> > >> > + maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); >> > + >> > /* Setup a metadata ControlList to output metadata. */ >> > libcameraMetadata_ = ControlList(controls::controls); >> > >> > @@ -1113,6 +1118,13 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) >> > { >> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); >> > >> > + /* >> > + * Ensure anything larger than the max gain code will not be passed to >> > + * DelayedControls. The AGC will correctly handle a lower gain returned >> > + * by the sensor, provided it knows the actual gain used. >> > + */ >> > + gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); >> > + >> >> Sounds ok to me though. >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> > /* GetVBlanking might clip exposure time to the fps limits. */ >> > Duration exposure = agcStatus->shutter_time; >> > int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_); >> > -- >> > 2.25.1 >> >
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 0ed41385018a..a72d516f84ee 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -173,6 +173,9 @@ private: /* Frame duration (1/fps) limits. */ Duration minFrameDuration_; Duration maxFrameDuration_; + + /* Maximum gain code for the sensor. */ + uint32_t maxSensorGainCode_; }; int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) @@ -357,6 +360,8 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo, return -1; } + maxSensorGainCode_ = sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>(); + /* Setup a metadata ControlList to output metadata. */ libcameraMetadata_ = ControlList(controls::controls); @@ -1113,6 +1118,13 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) { int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain); + /* + * Ensure anything larger than the max gain code will not be passed to + * DelayedControls. The AGC will correctly handle a lower gain returned + * by the sensor, provided it knows the actual gain used. + */ + gainCode = std::min<int32_t>(gainCode, maxSensorGainCode_); + /* GetVBlanking might clip exposure time to the fps limits. */ Duration exposure = agcStatus->shutter_time; int32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_, maxFrameDuration_);
Limit the gain code to the maximum value reported by the sensor controls when sending to DelayedControls. The AGC algorithm will handle a lower gain used by the sensor, provided provided it knows the actual gain used. This change ensures that DelayedControls will never report an unclipped gain used. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+)