| Message ID | 20220210114316.1151170-4-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jean-Michel Hautbois (2022-02-10 11:43:16) > Instead of having a local cached value for line duration, store it in > the IPASessionConfiguration::sensor structure. > While at it, configure the default analogue gain and shutter speed to > controlled fixed values. > > The latter is set to be 10ms as it will in most cases be close to the > one needed, making the AGC faster to converge. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 25 +++++++++++++++---------- > src/ipa/ipu3/algorithms/agc.h | 3 +-- > src/ipa/ipu3/ipa_context.cpp | 8 ++++++++ > src/ipa/ipu3/ipa_context.h | 4 ++++ > src/ipa/ipu3/ipu3.cpp | 27 ++++++++++++++------------- > 5 files changed, 42 insertions(+), 25 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index a929085c..95720ca6 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -71,7 +71,7 @@ static constexpr uint32_t kNumStartupFrames = 10; > static constexpr double kRelativeLuminanceTarget = 0.16; > > Agc::Agc() > - : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s), > + : frameCount_(0), minShutterSpeed_(0s), > maxShutterSpeed_(0s), filteredExposure_(0s) > { > } > @@ -85,11 +85,14 @@ Agc::Agc() > */ > int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > { > - stride_ = context.configuration.grid.stride; > + IPASessionConfiguration &configuration = context.configuration; > + IPAFrameContext &frameContext = context.frameContext; > + > + stride_ = configuration.grid.stride; > > /* \todo use the IPAContext to provide the limits */ > - lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s > - / configInfo.sensorInfo.pixelRate; > + configuration.sensor.lineDuration = configInfo.sensorInfo.lineLength * 1.0s > + / configInfo.sensorInfo.pixelRate; Can this be removed now that it's set in IPAIPU3::init()? What limits does the \todo talk about? (Or do they apply only to the calculations below this?) In fact I see it gets updated in IPAIPU3::configure() too, so I think this is just a leftover. > > minShutterSpeed_ = context.configuration.agc.minShutterSpeed; > maxShutterSpeed_ = std::min(context.configuration.agc.maxShutterSpeed, > @@ -99,8 +102,8 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain); > > /* Configure the default exposure and gain. */ > - context.frameContext.agc.gain = minAnalogueGain_; > - context.frameContext.agc.exposure = minShutterSpeed_ / lineDuration_; > + frameContext.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain); > + frameContext.agc.exposure = 10ms / configuration.sensor.lineDuration; > > return 0; > } > @@ -182,9 +185,11 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue) > * \param[in] yGain The gain calculated based on the relative luminance target > * \param[in] iqMeanGain The gain calculated based on the relative luminance target > */ > -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, > +void Agc::computeExposure(IPAContext &context, double yGain, > double iqMeanGain) > { > + const IPASessionConfiguration &configuration = context.configuration; > + IPAFrameContext &frameContext = context.frameContext; > /* Get the effective exposure and gain applied on the sensor. */ > uint32_t exposure = frameContext.sensor.exposure; > double analogueGain = frameContext.sensor.gain; > @@ -200,7 +205,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, > /* extracted from Rpi::Agc::computeTargetExposure */ > > /* Calculate the shutter time in seconds */ > - utils::Duration currentShutter = exposure * lineDuration_; > + utils::Duration currentShutter = exposure * configuration.sensor.lineDuration; > > /* > * Update the exposure value for the next computation using the values > @@ -247,7 +252,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, > << stepGain; > > /* Update the estimated exposure and gain. */ > - frameContext.agc.exposure = shutterTime / lineDuration_; > + frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration; > frameContext.agc.gain = stepGain; > } > > @@ -354,7 +359,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) > break; > } > > - computeExposure(context.frameContext, yGain, iqMeanGain); > + computeExposure(context, yGain, iqMeanGain); > frameCount_++; > } > > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h > index 84bfe045..ad705605 100644 > --- a/src/ipa/ipu3/algorithms/agc.h > +++ b/src/ipa/ipu3/algorithms/agc.h > @@ -34,7 +34,7 @@ private: > double measureBrightness(const ipu3_uapi_stats_3a *stats, > const ipu3_uapi_grid_config &grid) const; > utils::Duration filterExposure(utils::Duration currentExposure); > - void computeExposure(IPAFrameContext &frameContext, double yGain, > + void computeExposure(IPAContext &context, double yGain, > double iqMeanGain); > double estimateLuminance(IPAFrameContext &frameContext, > const ipu3_uapi_grid_config &grid, > @@ -43,7 +43,6 @@ private: > > uint64_t frameCount_; > > - utils::Duration lineDuration_; > utils::Duration minShutterSpeed_; > utils::Duration maxShutterSpeed_; > > diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp > index 86794ac1..9c4ec936 100644 > --- a/src/ipa/ipu3/ipa_context.cpp > +++ b/src/ipa/ipu3/ipa_context.cpp > @@ -86,6 +86,14 @@ namespace libcamera::ipa::ipu3 { > * \brief Maximum analogue gain supported with the configured sensor > */ > > +/** > + * \var IPASessionConfiguration::sensor > + * \brief Sensor-specific configuration of the IPA > + * > + * \var IPASessionConfiguration::sensor.lineDuration > + * \brief Line duration in microseconds > + */ > + > /** > * \var IPAFrameContext::agc > * \brief Context for the Automatic Gain Control algorithm > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > index c6dc0814..e7c49828 100644 > --- a/src/ipa/ipu3/ipa_context.h > +++ b/src/ipa/ipu3/ipa_context.h > @@ -31,6 +31,10 @@ struct IPASessionConfiguration { > double minAnalogueGain; > double maxAnalogueGain; > } agc; > + > + struct { > + utils::Duration lineDuration; > + } sensor; > }; > > struct IPAFrameContext { > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index c0c29416..b137c66c 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -173,8 +173,6 @@ private: > uint32_t minGain_; > uint32_t maxGain_; > > - utils::Duration lineDuration_; > - > /* Interface to the Camera Helper */ > std::unique_ptr<CameraSensorHelper> camHelper_; > > @@ -206,8 +204,8 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls) > * > * \todo take VBLANK into account for maximum shutter speed > */ > - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_; > - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_; > + context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration; > + context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration; > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > } > @@ -229,6 +227,7 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, > ControlInfoMap *ipaControls) > { > ControlInfoMap::Map controls{}; > + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); > > /* > * Compute exposure time limits by using line length and pixel rate > @@ -237,9 +236,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, > * microseconds. > */ > const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; > - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.get<std::micro>(); > - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.get<std::micro>(); > - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.get<std::micro>(); > + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; > + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; > + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; Seems a shame that we can't work with Durations more natively with Controls to make the control know the correct units, but that's not specific to this patch. > controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, > defExposure); > > @@ -293,6 +292,10 @@ int IPAIPU3::init(const IPASettings &settings, > return -ENODEV; > } > > + /* Clean context */ > + context_ = {}; > + context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate; > + > /* Construct our Algorithms */ > algorithms_.push_back(std::make_unique<algorithms::Agc>()); > algorithms_.push_back(std::make_unique<algorithms::Awb>()); > @@ -454,12 +457,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, > > defVBlank_ = itVBlank->second.def().get<int32_t>(); > > - /* Clean context at configuration */ > - context_ = {}; > - We're we relying on this to reset anything specifically when calling configure()? Or is this ok that it doesn't get reset now. (I think if we need to reset something at configure() it's probably better to be explicit about it anyway). If there's no expected regression with moving this clearing of the context during configure, then with the extra setting of sensor.lineDuration removed in the AGC: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > calculateBdsGrid(configInfo.bdsOutputSize); > > - lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; > + context_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; > > /* Update the camera controls using the new sensor settings. */ > updateControls(sensorInfo_, ctrls_, ipaControls); > @@ -620,6 +620,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, > [[maybe_unused]] int64_t frameTimestamp, > const ipu3_uapi_stats_3a *stats) > { > + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); > ControlList ctrls(controls::controls); > > for (auto const &algo : algorithms_) > @@ -628,14 +629,14 @@ void IPAIPU3::parseStatistics(unsigned int frame, > setControls(frame); > > /* \todo Use VBlank value calculated from each frame exposure. */ > - int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>(); > + int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration; > ctrls.set(controls::FrameDuration, frameDuration); > > ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain); > > ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK); > > - ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration_.get<std::micro>()); > + ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration); > > /* > * \todo The Metadata provides a path to getting extended data > -- > 2.32.0 >
Hi Kieran, On 10/02/2022 12:58, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2022-02-10 11:43:16) >> Instead of having a local cached value for line duration, store it in >> the IPASessionConfiguration::sensor structure. >> While at it, configure the default analogue gain and shutter speed to >> controlled fixed values. >> >> The latter is set to be 10ms as it will in most cases be close to the >> one needed, making the AGC faster to converge. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> src/ipa/ipu3/algorithms/agc.cpp | 25 +++++++++++++++---------- >> src/ipa/ipu3/algorithms/agc.h | 3 +-- >> src/ipa/ipu3/ipa_context.cpp | 8 ++++++++ >> src/ipa/ipu3/ipa_context.h | 4 ++++ >> src/ipa/ipu3/ipu3.cpp | 27 ++++++++++++++------------- >> 5 files changed, 42 insertions(+), 25 deletions(-) >> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp >> index a929085c..95720ca6 100644 >> --- a/src/ipa/ipu3/algorithms/agc.cpp >> +++ b/src/ipa/ipu3/algorithms/agc.cpp >> @@ -71,7 +71,7 @@ static constexpr uint32_t kNumStartupFrames = 10; >> static constexpr double kRelativeLuminanceTarget = 0.16; >> >> Agc::Agc() >> - : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s), >> + : frameCount_(0), minShutterSpeed_(0s), >> maxShutterSpeed_(0s), filteredExposure_(0s) >> { >> } >> @@ -85,11 +85,14 @@ Agc::Agc() >> */ >> int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) >> { >> - stride_ = context.configuration.grid.stride; >> + IPASessionConfiguration &configuration = context.configuration; >> + IPAFrameContext &frameContext = context.frameContext; >> + >> + stride_ = configuration.grid.stride; >> >> /* \todo use the IPAContext to provide the limits */ >> - lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s >> - / configInfo.sensorInfo.pixelRate; >> + configuration.sensor.lineDuration = configInfo.sensorInfo.lineLength * 1.0s >> + / configInfo.sensorInfo.pixelRate; > > Can this be removed now that it's set in IPAIPU3::init()? What limits > does the \todo talk about? (Or do they apply only to the calculations > below this?) > > In fact I see it gets updated in IPAIPU3::configure() too, so I think > this is just a leftover. > >> >> minShutterSpeed_ = context.configuration.agc.minShutterSpeed; >> maxShutterSpeed_ = std::min(context.configuration.agc.maxShutterSpeed, >> @@ -99,8 +102,8 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) >> maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain); >> >> /* Configure the default exposure and gain. */ >> - context.frameContext.agc.gain = minAnalogueGain_; >> - context.frameContext.agc.exposure = minShutterSpeed_ / lineDuration_; >> + frameContext.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain); >> + frameContext.agc.exposure = 10ms / configuration.sensor.lineDuration; >> >> return 0; >> } >> @@ -182,9 +185,11 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue) >> * \param[in] yGain The gain calculated based on the relative luminance target >> * \param[in] iqMeanGain The gain calculated based on the relative luminance target >> */ >> -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, >> +void Agc::computeExposure(IPAContext &context, double yGain, >> double iqMeanGain) >> { >> + const IPASessionConfiguration &configuration = context.configuration; >> + IPAFrameContext &frameContext = context.frameContext; >> /* Get the effective exposure and gain applied on the sensor. */ >> uint32_t exposure = frameContext.sensor.exposure; >> double analogueGain = frameContext.sensor.gain; >> @@ -200,7 +205,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, >> /* extracted from Rpi::Agc::computeTargetExposure */ >> >> /* Calculate the shutter time in seconds */ >> - utils::Duration currentShutter = exposure * lineDuration_; >> + utils::Duration currentShutter = exposure * configuration.sensor.lineDuration; >> >> /* >> * Update the exposure value for the next computation using the values >> @@ -247,7 +252,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, >> << stepGain; >> >> /* Update the estimated exposure and gain. */ >> - frameContext.agc.exposure = shutterTime / lineDuration_; >> + frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration; >> frameContext.agc.gain = stepGain; >> } >> >> @@ -354,7 +359,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) >> break; >> } >> >> - computeExposure(context.frameContext, yGain, iqMeanGain); >> + computeExposure(context, yGain, iqMeanGain); >> frameCount_++; >> } >> >> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h >> index 84bfe045..ad705605 100644 >> --- a/src/ipa/ipu3/algorithms/agc.h >> +++ b/src/ipa/ipu3/algorithms/agc.h >> @@ -34,7 +34,7 @@ private: >> double measureBrightness(const ipu3_uapi_stats_3a *stats, >> const ipu3_uapi_grid_config &grid) const; >> utils::Duration filterExposure(utils::Duration currentExposure); >> - void computeExposure(IPAFrameContext &frameContext, double yGain, >> + void computeExposure(IPAContext &context, double yGain, >> double iqMeanGain); >> double estimateLuminance(IPAFrameContext &frameContext, >> const ipu3_uapi_grid_config &grid, >> @@ -43,7 +43,6 @@ private: >> >> uint64_t frameCount_; >> >> - utils::Duration lineDuration_; >> utils::Duration minShutterSpeed_; >> utils::Duration maxShutterSpeed_; >> >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp >> index 86794ac1..9c4ec936 100644 >> --- a/src/ipa/ipu3/ipa_context.cpp >> +++ b/src/ipa/ipu3/ipa_context.cpp >> @@ -86,6 +86,14 @@ namespace libcamera::ipa::ipu3 { >> * \brief Maximum analogue gain supported with the configured sensor >> */ >> >> +/** >> + * \var IPASessionConfiguration::sensor >> + * \brief Sensor-specific configuration of the IPA >> + * >> + * \var IPASessionConfiguration::sensor.lineDuration >> + * \brief Line duration in microseconds >> + */ >> + >> /** >> * \var IPAFrameContext::agc >> * \brief Context for the Automatic Gain Control algorithm >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h >> index c6dc0814..e7c49828 100644 >> --- a/src/ipa/ipu3/ipa_context.h >> +++ b/src/ipa/ipu3/ipa_context.h >> @@ -31,6 +31,10 @@ struct IPASessionConfiguration { >> double minAnalogueGain; >> double maxAnalogueGain; >> } agc; >> + >> + struct { >> + utils::Duration lineDuration; >> + } sensor; >> }; >> >> struct IPAFrameContext { >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index c0c29416..b137c66c 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -173,8 +173,6 @@ private: >> uint32_t minGain_; >> uint32_t maxGain_; >> >> - utils::Duration lineDuration_; >> - >> /* Interface to the Camera Helper */ >> std::unique_ptr<CameraSensorHelper> camHelper_; >> >> @@ -206,8 +204,8 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls) >> * >> * \todo take VBLANK into account for maximum shutter speed >> */ >> - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_; >> - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_; >> + context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration; >> + context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration; >> context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); >> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); >> } >> @@ -229,6 +227,7 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, >> ControlInfoMap *ipaControls) >> { >> ControlInfoMap::Map controls{}; >> + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); >> >> /* >> * Compute exposure time limits by using line length and pixel rate >> @@ -237,9 +236,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, >> * microseconds. >> */ >> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; >> - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.get<std::micro>(); >> - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.get<std::micro>(); >> - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.get<std::micro>(); >> + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; >> + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; >> + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; > > Seems a shame that we can't work with Durations more natively with > Controls to make the control know the correct units, but that's not > specific to this patch. > >> controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, >> defExposure); >> >> @@ -293,6 +292,10 @@ int IPAIPU3::init(const IPASettings &settings, >> return -ENODEV; >> } >> >> + /* Clean context */ >> + context_ = {}; >> + context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate; >> + >> /* Construct our Algorithms */ >> algorithms_.push_back(std::make_unique<algorithms::Agc>()); >> algorithms_.push_back(std::make_unique<algorithms::Awb>()); >> @@ -454,12 +457,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, >> >> defVBlank_ = itVBlank->second.def().get<int32_t>(); >> >> - /* Clean context at configuration */ >> - context_ = {}; >> - > > We're we relying on this to reset anything specifically when calling > configure()? Or is this ok that it doesn't get reset now. (I think if we > need to reset something at configure() it's probably better to be > explicit about it anyway). > Is is moved in init() now, which, AFAIK, is called before configure ? > > If there's no expected regression with moving this clearing of the > context during configure, then with the extra setting of > sensor.lineDuration removed in the AGC: > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> calculateBdsGrid(configInfo.bdsOutputSize); >> >> - lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; >> + context_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; >> >> /* Update the camera controls using the new sensor settings. */ >> updateControls(sensorInfo_, ctrls_, ipaControls); >> @@ -620,6 +620,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, >> [[maybe_unused]] int64_t frameTimestamp, >> const ipu3_uapi_stats_3a *stats) >> { >> + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); >> ControlList ctrls(controls::controls); >> >> for (auto const &algo : algorithms_) >> @@ -628,14 +629,14 @@ void IPAIPU3::parseStatistics(unsigned int frame, >> setControls(frame); >> >> /* \todo Use VBlank value calculated from each frame exposure. */ >> - int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>(); >> + int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration; >> ctrls.set(controls::FrameDuration, frameDuration); >> >> ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain); >> >> ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK); >> >> - ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration_.get<std::micro>()); >> + ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration); >> >> /* >> * \todo The Metadata provides a path to getting extended data >> -- >> 2.32.0 >>
Quoting Jean-Michel Hautbois (2022-02-10 12:35:34) > Hi Kieran, > > On 10/02/2022 12:58, Kieran Bingham wrote: > > Quoting Jean-Michel Hautbois (2022-02-10 11:43:16) > >> Instead of having a local cached value for line duration, store it in > >> the IPASessionConfiguration::sensor structure. > >> While at it, configure the default analogue gain and shutter speed to > >> controlled fixed values. > >> > >> The latter is set to be 10ms as it will in most cases be close to the > >> one needed, making the AGC faster to converge. > >> > >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > >> --- > >> src/ipa/ipu3/algorithms/agc.cpp | 25 +++++++++++++++---------- > >> src/ipa/ipu3/algorithms/agc.h | 3 +-- > >> src/ipa/ipu3/ipa_context.cpp | 8 ++++++++ > >> src/ipa/ipu3/ipa_context.h | 4 ++++ > >> src/ipa/ipu3/ipu3.cpp | 27 ++++++++++++++------------- > >> 5 files changed, 42 insertions(+), 25 deletions(-) > >> > >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > >> index a929085c..95720ca6 100644 > >> --- a/src/ipa/ipu3/algorithms/agc.cpp > >> +++ b/src/ipa/ipu3/algorithms/agc.cpp > >> @@ -71,7 +71,7 @@ static constexpr uint32_t kNumStartupFrames = 10; > >> static constexpr double kRelativeLuminanceTarget = 0.16; > >> > >> Agc::Agc() > >> - : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s), > >> + : frameCount_(0), minShutterSpeed_(0s), > >> maxShutterSpeed_(0s), filteredExposure_(0s) > >> { > >> } > >> @@ -85,11 +85,14 @@ Agc::Agc() > >> */ > >> int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > >> { > >> - stride_ = context.configuration.grid.stride; > >> + IPASessionConfiguration &configuration = context.configuration; > >> + IPAFrameContext &frameContext = context.frameContext; > >> + > >> + stride_ = configuration.grid.stride; > >> > >> /* \todo use the IPAContext to provide the limits */ > >> - lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s > >> - / configInfo.sensorInfo.pixelRate; > >> + configuration.sensor.lineDuration = configInfo.sensorInfo.lineLength * 1.0s > >> + / configInfo.sensorInfo.pixelRate; > > > > Can this be removed now that it's set in IPAIPU3::init()? What limits > > does the \todo talk about? (Or do they apply only to the calculations > > below this?) > > > > In fact I see it gets updated in IPAIPU3::configure() too, so I think > > this is just a leftover. > > > >> > >> minShutterSpeed_ = context.configuration.agc.minShutterSpeed; > >> maxShutterSpeed_ = std::min(context.configuration.agc.maxShutterSpeed, > >> @@ -99,8 +102,8 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) > >> maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain); > >> > >> /* Configure the default exposure and gain. */ > >> - context.frameContext.agc.gain = minAnalogueGain_; > >> - context.frameContext.agc.exposure = minShutterSpeed_ / lineDuration_; > >> + frameContext.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain); > >> + frameContext.agc.exposure = 10ms / configuration.sensor.lineDuration; > >> > >> return 0; > >> } > >> @@ -182,9 +185,11 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue) > >> * \param[in] yGain The gain calculated based on the relative luminance target > >> * \param[in] iqMeanGain The gain calculated based on the relative luminance target > >> */ > >> -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, > >> +void Agc::computeExposure(IPAContext &context, double yGain, > >> double iqMeanGain) > >> { > >> + const IPASessionConfiguration &configuration = context.configuration; > >> + IPAFrameContext &frameContext = context.frameContext; > >> /* Get the effective exposure and gain applied on the sensor. */ > >> uint32_t exposure = frameContext.sensor.exposure; > >> double analogueGain = frameContext.sensor.gain; > >> @@ -200,7 +205,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, > >> /* extracted from Rpi::Agc::computeTargetExposure */ > >> > >> /* Calculate the shutter time in seconds */ > >> - utils::Duration currentShutter = exposure * lineDuration_; > >> + utils::Duration currentShutter = exposure * configuration.sensor.lineDuration; > >> > >> /* > >> * Update the exposure value for the next computation using the values > >> @@ -247,7 +252,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, > >> << stepGain; > >> > >> /* Update the estimated exposure and gain. */ > >> - frameContext.agc.exposure = shutterTime / lineDuration_; > >> + frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration; > >> frameContext.agc.gain = stepGain; > >> } > >> > >> @@ -354,7 +359,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) > >> break; > >> } > >> > >> - computeExposure(context.frameContext, yGain, iqMeanGain); > >> + computeExposure(context, yGain, iqMeanGain); > >> frameCount_++; > >> } > >> > >> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h > >> index 84bfe045..ad705605 100644 > >> --- a/src/ipa/ipu3/algorithms/agc.h > >> +++ b/src/ipa/ipu3/algorithms/agc.h > >> @@ -34,7 +34,7 @@ private: > >> double measureBrightness(const ipu3_uapi_stats_3a *stats, > >> const ipu3_uapi_grid_config &grid) const; > >> utils::Duration filterExposure(utils::Duration currentExposure); > >> - void computeExposure(IPAFrameContext &frameContext, double yGain, > >> + void computeExposure(IPAContext &context, double yGain, > >> double iqMeanGain); > >> double estimateLuminance(IPAFrameContext &frameContext, > >> const ipu3_uapi_grid_config &grid, > >> @@ -43,7 +43,6 @@ private: > >> > >> uint64_t frameCount_; > >> > >> - utils::Duration lineDuration_; > >> utils::Duration minShutterSpeed_; > >> utils::Duration maxShutterSpeed_; > >> > >> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp > >> index 86794ac1..9c4ec936 100644 > >> --- a/src/ipa/ipu3/ipa_context.cpp > >> +++ b/src/ipa/ipu3/ipa_context.cpp > >> @@ -86,6 +86,14 @@ namespace libcamera::ipa::ipu3 { > >> * \brief Maximum analogue gain supported with the configured sensor > >> */ > >> > >> +/** > >> + * \var IPASessionConfiguration::sensor > >> + * \brief Sensor-specific configuration of the IPA > >> + * > >> + * \var IPASessionConfiguration::sensor.lineDuration > >> + * \brief Line duration in microseconds > >> + */ > >> + > >> /** > >> * \var IPAFrameContext::agc > >> * \brief Context for the Automatic Gain Control algorithm > >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h > >> index c6dc0814..e7c49828 100644 > >> --- a/src/ipa/ipu3/ipa_context.h > >> +++ b/src/ipa/ipu3/ipa_context.h > >> @@ -31,6 +31,10 @@ struct IPASessionConfiguration { > >> double minAnalogueGain; > >> double maxAnalogueGain; > >> } agc; > >> + > >> + struct { > >> + utils::Duration lineDuration; > >> + } sensor; > >> }; > >> > >> struct IPAFrameContext { > >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > >> index c0c29416..b137c66c 100644 > >> --- a/src/ipa/ipu3/ipu3.cpp > >> +++ b/src/ipa/ipu3/ipu3.cpp > >> @@ -173,8 +173,6 @@ private: > >> uint32_t minGain_; > >> uint32_t maxGain_; > >> > >> - utils::Duration lineDuration_; > >> - > >> /* Interface to the Camera Helper */ > >> std::unique_ptr<CameraSensorHelper> camHelper_; > >> > >> @@ -206,8 +204,8 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls) > >> * > >> * \todo take VBLANK into account for maximum shutter speed > >> */ > >> - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_; > >> - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_; > >> + context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration; > >> + context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration; > >> context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > >> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > >> } > >> @@ -229,6 +227,7 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, > >> ControlInfoMap *ipaControls) > >> { > >> ControlInfoMap::Map controls{}; > >> + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); > >> > >> /* > >> * Compute exposure time limits by using line length and pixel rate > >> @@ -237,9 +236,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, > >> * microseconds. > >> */ > >> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; > >> - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.get<std::micro>(); > >> - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.get<std::micro>(); > >> - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.get<std::micro>(); > >> + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; > >> + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; > >> + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; > > > > Seems a shame that we can't work with Durations more natively with > > Controls to make the control know the correct units, but that's not > > specific to this patch. > > > >> controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, > >> defExposure); > >> > >> @@ -293,6 +292,10 @@ int IPAIPU3::init(const IPASettings &settings, > >> return -ENODEV; > >> } > >> > >> + /* Clean context */ > >> + context_ = {}; > >> + context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate; > >> + > >> /* Construct our Algorithms */ > >> algorithms_.push_back(std::make_unique<algorithms::Agc>()); > >> algorithms_.push_back(std::make_unique<algorithms::Awb>()); > >> @@ -454,12 +457,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, > >> > >> defVBlank_ = itVBlank->second.def().get<int32_t>(); > >> > >> - /* Clean context at configuration */ > >> - context_ = {}; > >> - > > > > We're we relying on this to reset anything specifically when calling > > configure()? Or is this ok that it doesn't get reset now. (I think if we > > need to reset something at configure() it's probably better to be > > explicit about it anyway). > > > > Is is moved in init() now, which, AFAIK, is called before configure ? Yes, that's fine, but was there any reason it was called in configure so that something gets reset in between configures rather than init() when it was added? I.e. is anything after configure relying on the fact that it is reset on every configure() call. (I.e. is there anything relying on if (value == 0) value = x; during the algorithm configure() calls or otherwise?) For example, why is this set in init() now, rather than the contstructor? Is it that we need to make sure the FrameContext is cleared down on any reconfiguration? (Which perhaps will move out when it's moved to a per-frame-contexts anyway). > > > > > If there's no expected regression with moving this clearing of the > > context during configure, then with the extra setting of > > sensor.lineDuration removed in the AGC: > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > >> calculateBdsGrid(configInfo.bdsOutputSize); > >> > >> - lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; > >> + context_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; > >> > >> /* Update the camera controls using the new sensor settings. */ > >> updateControls(sensorInfo_, ctrls_, ipaControls); > >> @@ -620,6 +620,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, > >> [[maybe_unused]] int64_t frameTimestamp, > >> const ipu3_uapi_stats_3a *stats) > >> { > >> + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); > >> ControlList ctrls(controls::controls); > >> > >> for (auto const &algo : algorithms_) > >> @@ -628,14 +629,14 @@ void IPAIPU3::parseStatistics(unsigned int frame, > >> setControls(frame); > >> > >> /* \todo Use VBlank value calculated from each frame exposure. */ > >> - int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>(); > >> + int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration; > >> ctrls.set(controls::FrameDuration, frameDuration); > >> > >> ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain); > >> > >> ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK); > >> > >> - ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration_.get<std::micro>()); > >> + ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration); > >> > >> /* > >> * \todo The Metadata provides a path to getting extended data > >> -- > >> 2.32.0 > >>
Hi Kieran, On 10/02/2022 14:12, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2022-02-10 12:35:34) >> Hi Kieran, >> >> On 10/02/2022 12:58, Kieran Bingham wrote: >>> Quoting Jean-Michel Hautbois (2022-02-10 11:43:16) >>>> Instead of having a local cached value for line duration, store it in >>>> the IPASessionConfiguration::sensor structure. >>>> While at it, configure the default analogue gain and shutter speed to >>>> controlled fixed values. >>>> >>>> The latter is set to be 10ms as it will in most cases be close to the >>>> one needed, making the AGC faster to converge. >>>> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >>>> --- >>>> src/ipa/ipu3/algorithms/agc.cpp | 25 +++++++++++++++---------- >>>> src/ipa/ipu3/algorithms/agc.h | 3 +-- >>>> src/ipa/ipu3/ipa_context.cpp | 8 ++++++++ >>>> src/ipa/ipu3/ipa_context.h | 4 ++++ >>>> src/ipa/ipu3/ipu3.cpp | 27 ++++++++++++++------------- >>>> 5 files changed, 42 insertions(+), 25 deletions(-) >>>> >>>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp >>>> index a929085c..95720ca6 100644 >>>> --- a/src/ipa/ipu3/algorithms/agc.cpp >>>> +++ b/src/ipa/ipu3/algorithms/agc.cpp >>>> @@ -71,7 +71,7 @@ static constexpr uint32_t kNumStartupFrames = 10; >>>> static constexpr double kRelativeLuminanceTarget = 0.16; >>>> >>>> Agc::Agc() >>>> - : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s), >>>> + : frameCount_(0), minShutterSpeed_(0s), >>>> maxShutterSpeed_(0s), filteredExposure_(0s) >>>> { >>>> } >>>> @@ -85,11 +85,14 @@ Agc::Agc() >>>> */ >>>> int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) >>>> { >>>> - stride_ = context.configuration.grid.stride; >>>> + IPASessionConfiguration &configuration = context.configuration; >>>> + IPAFrameContext &frameContext = context.frameContext; >>>> + >>>> + stride_ = configuration.grid.stride; >>>> >>>> /* \todo use the IPAContext to provide the limits */ >>>> - lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s >>>> - / configInfo.sensorInfo.pixelRate; >>>> + configuration.sensor.lineDuration = configInfo.sensorInfo.lineLength * 1.0s >>>> + / configInfo.sensorInfo.pixelRate; >>> >>> Can this be removed now that it's set in IPAIPU3::init()? What limits >>> does the \todo talk about? (Or do they apply only to the calculations >>> below this?) >>> >>> In fact I see it gets updated in IPAIPU3::configure() too, so I think >>> this is just a leftover. >>> >>>> >>>> minShutterSpeed_ = context.configuration.agc.minShutterSpeed; >>>> maxShutterSpeed_ = std::min(context.configuration.agc.maxShutterSpeed, >>>> @@ -99,8 +102,8 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) >>>> maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain); >>>> >>>> /* Configure the default exposure and gain. */ >>>> - context.frameContext.agc.gain = minAnalogueGain_; >>>> - context.frameContext.agc.exposure = minShutterSpeed_ / lineDuration_; >>>> + frameContext.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain); >>>> + frameContext.agc.exposure = 10ms / configuration.sensor.lineDuration; >>>> >>>> return 0; >>>> } >>>> @@ -182,9 +185,11 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue) >>>> * \param[in] yGain The gain calculated based on the relative luminance target >>>> * \param[in] iqMeanGain The gain calculated based on the relative luminance target >>>> */ >>>> -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, >>>> +void Agc::computeExposure(IPAContext &context, double yGain, >>>> double iqMeanGain) >>>> { >>>> + const IPASessionConfiguration &configuration = context.configuration; >>>> + IPAFrameContext &frameContext = context.frameContext; >>>> /* Get the effective exposure and gain applied on the sensor. */ >>>> uint32_t exposure = frameContext.sensor.exposure; >>>> double analogueGain = frameContext.sensor.gain; >>>> @@ -200,7 +205,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, >>>> /* extracted from Rpi::Agc::computeTargetExposure */ >>>> >>>> /* Calculate the shutter time in seconds */ >>>> - utils::Duration currentShutter = exposure * lineDuration_; >>>> + utils::Duration currentShutter = exposure * configuration.sensor.lineDuration; >>>> >>>> /* >>>> * Update the exposure value for the next computation using the values >>>> @@ -247,7 +252,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, >>>> << stepGain; >>>> >>>> /* Update the estimated exposure and gain. */ >>>> - frameContext.agc.exposure = shutterTime / lineDuration_; >>>> + frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration; >>>> frameContext.agc.gain = stepGain; >>>> } >>>> >>>> @@ -354,7 +359,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) >>>> break; >>>> } >>>> >>>> - computeExposure(context.frameContext, yGain, iqMeanGain); >>>> + computeExposure(context, yGain, iqMeanGain); >>>> frameCount_++; >>>> } >>>> >>>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h >>>> index 84bfe045..ad705605 100644 >>>> --- a/src/ipa/ipu3/algorithms/agc.h >>>> +++ b/src/ipa/ipu3/algorithms/agc.h >>>> @@ -34,7 +34,7 @@ private: >>>> double measureBrightness(const ipu3_uapi_stats_3a *stats, >>>> const ipu3_uapi_grid_config &grid) const; >>>> utils::Duration filterExposure(utils::Duration currentExposure); >>>> - void computeExposure(IPAFrameContext &frameContext, double yGain, >>>> + void computeExposure(IPAContext &context, double yGain, >>>> double iqMeanGain); >>>> double estimateLuminance(IPAFrameContext &frameContext, >>>> const ipu3_uapi_grid_config &grid, >>>> @@ -43,7 +43,6 @@ private: >>>> >>>> uint64_t frameCount_; >>>> >>>> - utils::Duration lineDuration_; >>>> utils::Duration minShutterSpeed_; >>>> utils::Duration maxShutterSpeed_; >>>> >>>> diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp >>>> index 86794ac1..9c4ec936 100644 >>>> --- a/src/ipa/ipu3/ipa_context.cpp >>>> +++ b/src/ipa/ipu3/ipa_context.cpp >>>> @@ -86,6 +86,14 @@ namespace libcamera::ipa::ipu3 { >>>> * \brief Maximum analogue gain supported with the configured sensor >>>> */ >>>> >>>> +/** >>>> + * \var IPASessionConfiguration::sensor >>>> + * \brief Sensor-specific configuration of the IPA >>>> + * >>>> + * \var IPASessionConfiguration::sensor.lineDuration >>>> + * \brief Line duration in microseconds >>>> + */ >>>> + >>>> /** >>>> * \var IPAFrameContext::agc >>>> * \brief Context for the Automatic Gain Control algorithm >>>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h >>>> index c6dc0814..e7c49828 100644 >>>> --- a/src/ipa/ipu3/ipa_context.h >>>> +++ b/src/ipa/ipu3/ipa_context.h >>>> @@ -31,6 +31,10 @@ struct IPASessionConfiguration { >>>> double minAnalogueGain; >>>> double maxAnalogueGain; >>>> } agc; >>>> + >>>> + struct { >>>> + utils::Duration lineDuration; >>>> + } sensor; >>>> }; >>>> >>>> struct IPAFrameContext { >>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >>>> index c0c29416..b137c66c 100644 >>>> --- a/src/ipa/ipu3/ipu3.cpp >>>> +++ b/src/ipa/ipu3/ipu3.cpp >>>> @@ -173,8 +173,6 @@ private: >>>> uint32_t minGain_; >>>> uint32_t maxGain_; >>>> >>>> - utils::Duration lineDuration_; >>>> - >>>> /* Interface to the Camera Helper */ >>>> std::unique_ptr<CameraSensorHelper> camHelper_; >>>> >>>> @@ -206,8 +204,8 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls) >>>> * >>>> * \todo take VBLANK into account for maximum shutter speed >>>> */ >>>> - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_; >>>> - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_; >>>> + context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration; >>>> + context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration; >>>> context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); >>>> context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); >>>> } >>>> @@ -229,6 +227,7 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, >>>> ControlInfoMap *ipaControls) >>>> { >>>> ControlInfoMap::Map controls{}; >>>> + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); >>>> >>>> /* >>>> * Compute exposure time limits by using line length and pixel rate >>>> @@ -237,9 +236,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, >>>> * microseconds. >>>> */ >>>> const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; >>>> - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.get<std::micro>(); >>>> - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.get<std::micro>(); >>>> - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.get<std::micro>(); >>>> + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; >>>> + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; >>>> + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; >>> >>> Seems a shame that we can't work with Durations more natively with >>> Controls to make the control know the correct units, but that's not >>> specific to this patch. >>> >>>> controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, >>>> defExposure); >>>> >>>> @@ -293,6 +292,10 @@ int IPAIPU3::init(const IPASettings &settings, >>>> return -ENODEV; >>>> } >>>> >>>> + /* Clean context */ >>>> + context_ = {}; >>>> + context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate; >>>> + >>>> /* Construct our Algorithms */ >>>> algorithms_.push_back(std::make_unique<algorithms::Agc>()); >>>> algorithms_.push_back(std::make_unique<algorithms::Awb>()); >>>> @@ -454,12 +457,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, >>>> >>>> defVBlank_ = itVBlank->second.def().get<int32_t>(); >>>> >>>> - /* Clean context at configuration */ >>>> - context_ = {}; >>>> - >>> >>> We're we relying on this to reset anything specifically when calling >>> configure()? Or is this ok that it doesn't get reset now. (I think if we >>> need to reset something at configure() it's probably better to be >>> explicit about it anyway). >>> >> >> Is is moved in init() now, which, AFAIK, is called before configure ? > > Yes, that's fine, but was there any reason it was called in configure so > that something gets reset in between configures rather than init() when > it was added? > > I.e. is anything after configure relying on the fact that it is reset on > every configure() call. > > (I.e. is there anything relying on if (value == 0) value = x; during the > algorithm configure() calls or otherwise?) > > For example, why is this set in init() now, rather than the > contstructor? > > Is it that we need to make sure the FrameContext is cleared down on any > reconfiguration? (Which perhaps will move out when it's moved to a > per-frame-contexts anyway). It should probably be reset in both init() and configure, to be sure we reset frameContext at the very least on each configure() call. I will send v4 with this. >> >>> >>> If there's no expected regression with moving this clearing of the >>> context during configure, then with the extra setting of >>> sensor.lineDuration removed in the AGC: >>> >>> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>> >>> >>>> calculateBdsGrid(configInfo.bdsOutputSize); >>>> >>>> - lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; >>>> + context_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; >>>> >>>> /* Update the camera controls using the new sensor settings. */ >>>> updateControls(sensorInfo_, ctrls_, ipaControls); >>>> @@ -620,6 +620,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, >>>> [[maybe_unused]] int64_t frameTimestamp, >>>> const ipu3_uapi_stats_3a *stats) >>>> { >>>> + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); >>>> ControlList ctrls(controls::controls); >>>> >>>> for (auto const &algo : algorithms_) >>>> @@ -628,14 +629,14 @@ void IPAIPU3::parseStatistics(unsigned int frame, >>>> setControls(frame); >>>> >>>> /* \todo Use VBlank value calculated from each frame exposure. */ >>>> - int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>(); >>>> + int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration; >>>> ctrls.set(controls::FrameDuration, frameDuration); >>>> >>>> ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain); >>>> >>>> ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK); >>>> >>>> - ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration_.get<std::micro>()); >>>> + ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration); >>>> >>>> /* >>>> * \todo The Metadata provides a path to getting extended data >>>> -- >>>> 2.32.0 >>>>
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index a929085c..95720ca6 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -71,7 +71,7 @@ static constexpr uint32_t kNumStartupFrames = 10; static constexpr double kRelativeLuminanceTarget = 0.16; Agc::Agc() - : frameCount_(0), lineDuration_(0s), minShutterSpeed_(0s), + : frameCount_(0), minShutterSpeed_(0s), maxShutterSpeed_(0s), filteredExposure_(0s) { } @@ -85,11 +85,14 @@ Agc::Agc() */ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) { - stride_ = context.configuration.grid.stride; + IPASessionConfiguration &configuration = context.configuration; + IPAFrameContext &frameContext = context.frameContext; + + stride_ = configuration.grid.stride; /* \todo use the IPAContext to provide the limits */ - lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s - / configInfo.sensorInfo.pixelRate; + configuration.sensor.lineDuration = configInfo.sensorInfo.lineLength * 1.0s + / configInfo.sensorInfo.pixelRate; minShutterSpeed_ = context.configuration.agc.minShutterSpeed; maxShutterSpeed_ = std::min(context.configuration.agc.maxShutterSpeed, @@ -99,8 +102,8 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo) maxAnalogueGain_ = std::min(context.configuration.agc.maxAnalogueGain, kMaxAnalogueGain); /* Configure the default exposure and gain. */ - context.frameContext.agc.gain = minAnalogueGain_; - context.frameContext.agc.exposure = minShutterSpeed_ / lineDuration_; + frameContext.agc.gain = std::max(minAnalogueGain_, kMinAnalogueGain); + frameContext.agc.exposure = 10ms / configuration.sensor.lineDuration; return 0; } @@ -182,9 +185,11 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue) * \param[in] yGain The gain calculated based on the relative luminance target * \param[in] iqMeanGain The gain calculated based on the relative luminance target */ -void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, +void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain) { + const IPASessionConfiguration &configuration = context.configuration; + IPAFrameContext &frameContext = context.frameContext; /* Get the effective exposure and gain applied on the sensor. */ uint32_t exposure = frameContext.sensor.exposure; double analogueGain = frameContext.sensor.gain; @@ -200,7 +205,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, /* extracted from Rpi::Agc::computeTargetExposure */ /* Calculate the shutter time in seconds */ - utils::Duration currentShutter = exposure * lineDuration_; + utils::Duration currentShutter = exposure * configuration.sensor.lineDuration; /* * Update the exposure value for the next computation using the values @@ -247,7 +252,7 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain, << stepGain; /* Update the estimated exposure and gain. */ - frameContext.agc.exposure = shutterTime / lineDuration_; + frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration; frameContext.agc.gain = stepGain; } @@ -354,7 +359,7 @@ void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats) break; } - computeExposure(context.frameContext, yGain, iqMeanGain); + computeExposure(context, yGain, iqMeanGain); frameCount_++; } diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h index 84bfe045..ad705605 100644 --- a/src/ipa/ipu3/algorithms/agc.h +++ b/src/ipa/ipu3/algorithms/agc.h @@ -34,7 +34,7 @@ private: double measureBrightness(const ipu3_uapi_stats_3a *stats, const ipu3_uapi_grid_config &grid) const; utils::Duration filterExposure(utils::Duration currentExposure); - void computeExposure(IPAFrameContext &frameContext, double yGain, + void computeExposure(IPAContext &context, double yGain, double iqMeanGain); double estimateLuminance(IPAFrameContext &frameContext, const ipu3_uapi_grid_config &grid, @@ -43,7 +43,6 @@ private: uint64_t frameCount_; - utils::Duration lineDuration_; utils::Duration minShutterSpeed_; utils::Duration maxShutterSpeed_; diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp index 86794ac1..9c4ec936 100644 --- a/src/ipa/ipu3/ipa_context.cpp +++ b/src/ipa/ipu3/ipa_context.cpp @@ -86,6 +86,14 @@ namespace libcamera::ipa::ipu3 { * \brief Maximum analogue gain supported with the configured sensor */ +/** + * \var IPASessionConfiguration::sensor + * \brief Sensor-specific configuration of the IPA + * + * \var IPASessionConfiguration::sensor.lineDuration + * \brief Line duration in microseconds + */ + /** * \var IPAFrameContext::agc * \brief Context for the Automatic Gain Control algorithm diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h index c6dc0814..e7c49828 100644 --- a/src/ipa/ipu3/ipa_context.h +++ b/src/ipa/ipu3/ipa_context.h @@ -31,6 +31,10 @@ struct IPASessionConfiguration { double minAnalogueGain; double maxAnalogueGain; } agc; + + struct { + utils::Duration lineDuration; + } sensor; }; struct IPAFrameContext { diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index c0c29416..b137c66c 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -173,8 +173,6 @@ private: uint32_t minGain_; uint32_t maxGain_; - utils::Duration lineDuration_; - /* Interface to the Camera Helper */ std::unique_ptr<CameraSensorHelper> camHelper_; @@ -206,8 +204,8 @@ void IPAIPU3::updateSessionConfiguration(const ControlInfoMap &sensorControls) * * \todo take VBLANK into account for maximum shutter speed */ - context_.configuration.agc.minShutterSpeed = minExposure * lineDuration_; - context_.configuration.agc.maxShutterSpeed = maxExposure * lineDuration_; + context_.configuration.agc.minShutterSpeed = minExposure * context_.configuration.sensor.lineDuration; + context_.configuration.agc.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration; context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); } @@ -229,6 +227,7 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, ControlInfoMap *ipaControls) { ControlInfoMap::Map controls{}; + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); /* * Compute exposure time limits by using line length and pixel rate @@ -237,9 +236,9 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo, * microseconds. */ const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; - int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration_.get<std::micro>(); - int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration_.get<std::micro>(); - int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration_.get<std::micro>(); + int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration; + int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration; + int32_t defExposure = v4l2Exposure.def().get<int32_t>() * lineDuration; controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure, defExposure); @@ -293,6 +292,10 @@ int IPAIPU3::init(const IPASettings &settings, return -ENODEV; } + /* Clean context */ + context_ = {}; + context_.configuration.sensor.lineDuration = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate; + /* Construct our Algorithms */ algorithms_.push_back(std::make_unique<algorithms::Agc>()); algorithms_.push_back(std::make_unique<algorithms::Awb>()); @@ -454,12 +457,9 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, defVBlank_ = itVBlank->second.def().get<int32_t>(); - /* Clean context at configuration */ - context_ = {}; - calculateBdsGrid(configInfo.bdsOutputSize); - lineDuration_ = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; + context_.configuration.sensor.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate; /* Update the camera controls using the new sensor settings. */ updateControls(sensorInfo_, ctrls_, ipaControls); @@ -620,6 +620,7 @@ void IPAIPU3::parseStatistics(unsigned int frame, [[maybe_unused]] int64_t frameTimestamp, const ipu3_uapi_stats_3a *stats) { + double lineDuration = context_.configuration.sensor.lineDuration.get<std::micro>(); ControlList ctrls(controls::controls); for (auto const &algo : algorithms_) @@ -628,14 +629,14 @@ void IPAIPU3::parseStatistics(unsigned int frame, setControls(frame); /* \todo Use VBlank value calculated from each frame exposure. */ - int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration_.get<std::micro>(); + int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) * lineDuration; ctrls.set(controls::FrameDuration, frameDuration); ctrls.set(controls::AnalogueGain, context_.frameContext.sensor.gain); ctrls.set(controls::ColourTemperature, context_.frameContext.awb.temperatureK); - ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration_.get<std::micro>()); + ctrls.set(controls::ExposureTime, context_.frameContext.sensor.exposure * lineDuration); /* * \todo The Metadata provides a path to getting extended data
Instead of having a local cached value for line duration, store it in the IPASessionConfiguration::sensor structure. While at it, configure the default analogue gain and shutter speed to controlled fixed values. The latter is set to be 10ms as it will in most cases be close to the one needed, making the AGC faster to converge. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 25 +++++++++++++++---------- src/ipa/ipu3/algorithms/agc.h | 3 +-- src/ipa/ipu3/ipa_context.cpp | 8 ++++++++ src/ipa/ipu3/ipa_context.h | 4 ++++ src/ipa/ipu3/ipu3.cpp | 27 ++++++++++++++------------- 5 files changed, 42 insertions(+), 25 deletions(-)