[libcamera-devel] [PATCH v3 05/13] ipa: rkisp1: agc: Support raw capture

[Laurent Pinchart]

Hi Jacopo,

On Wed, Oct 26, 2022 at 04:52:30PM +0200, Jacopo Mondi wrote:
> On Mon, Oct 24, 2022 at 03:03:48AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > Support raw capture by allowing manual control of the exposure time and
> > analogue gain.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp | 51 ++++++++++++++++++++-----------
> >  src/ipa/rkisp1/algorithms/agc.h   |  2 ++
> >  2 files changed, 36 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 377a3e8a0efd..973965babed2 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -62,6 +62,7 @@ static constexpr double kRelativeLuminanceTarget = 0.4;
> >  Agc::Agc()
> >  	: frameCount_(0), numCells_(0), numHistBins_(0), filteredExposure_(0s)
> >  {
> > +	supportsRaw_ = true;
> >  }
> >
> >  /**
> > @@ -81,7 +82,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >  		10ms / context.configuration.sensor.lineDuration;
> >  	context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
> >  	context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> > -	context.activeState.agc.autoEnabled = true;
> > +	context.activeState.agc.autoEnabled = !context.configuration.raw;
> >
> >  	/*
> >  	 * According to the RkISP1 documentation:
> > @@ -123,12 +124,14 @@ void Agc::queueRequest(IPAContext &context,
> >  {
> >  	auto &agc = context.activeState.agc;
> >
> > -	const auto &agcEnable = controls.get(controls::AeEnable);
> > -	if (agcEnable && *agcEnable != agc.autoEnabled) {
> > -		agc.autoEnabled = *agcEnable;
> > +	if (!context.configuration.raw) {
> 
> I wonder if we shouldn't complain loud when the application tries to
> enable AutoExposure with a RAW configuration..

Why should we complain more loudly than for any other invalid
combination of control values ?

> > +		const auto &agcEnable = controls.get(controls::AeEnable);
> > +		if (agcEnable && *agcEnable != agc.autoEnabled) {
> > +			agc.autoEnabled = *agcEnable;
> >
> > -		LOG(RkISP1Agc, Debug)
> > -			<< (*agcEnable ? "Enabling" : "Disabling") << " AGC";
> > +			LOG(RkISP1Agc, Debug)
> > +				<< (*agcEnable ? "Enabling" : "Disabling") << " AGC";
> > +		}
> >  	}
> >
> >  	const auto &exposure = controls.get(controls::ExposureTime);
> > @@ -160,6 +163,9 @@ void Agc::queueRequest(IPAContext &context,
> >  void Agc::prepare(IPAContext &context, const uint32_t frame,
> >  		  IPAFrameContext &frameContext, rkisp1_params_cfg *params)
> >  {
> > +	if (!params)
> > +		return;
> > +
> 
> I might have missed where params is set to nullptr in the IPA module.
> 
> 
> void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
> {
> 	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> 	rkisp1_params_cfg *params =
> 		reinterpret_cast<rkisp1_params_cfg *>(
> 			mappedBuffers_.at(bufferId).planes()[0].data());
> 
> 	/* Prepare parameters buffer. */
> 	memset(params, 0, sizeof(*params));
> 
> 	for (auto const &algo : algorithms())
> 		algo->prepare(context_, frame, frameContext, params);
> 
> Does 'params' need to be treated as 'stats' in
> IPARkISP1::processStatsBuffer() (ie set to nullptr if
> configuration.raw) ?

No, you're right, it can't be null, so I'll drop this check.

> >  	if (frameContext.agc.autoEnabled) {
> >  		frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
> >  		frameContext.agc.gain = context.activeState.agc.automatic.gain;
> > @@ -367,6 +373,22 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const
> >  	return histogram.interQuantileMean(0.98, 1.0);
> >  }
> >
> > +void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > +		       ControlList &metadata)
> > +{
> > +	utils::Duration exposureTime = context.configuration.sensor.lineDuration
> > +				     * frameContext.sensor.exposure;
> > +	metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> > +	metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
> > +
> > +	/* \todo Use VBlank value calculated from each frame exposure. */
> > +	uint32_t vTotal = context.configuration.sensor.size.height
> > +			+ context.configuration.sensor.defVBlank;
> > +	utils::Duration frameDuration = context.configuration.sensor.lineDuration
> > +				      * vTotal;
> > +	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> > +}
> > +
> >  /**
> >   * \brief Process RkISP1 statistics, and run AGC operations
> >   * \param[in] context The shared IPA context
> > @@ -382,6 +404,11 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >  		  IPAFrameContext &frameContext, const rkisp1_stat_buffer *stats,
> >  		  ControlList &metadata)
> >  {
> > +	if (!stats) {
> > +		fillMetadata(context, frameContext, metadata);
> > +		return;
> > +	}
> > +
> >  	/*
> >  	 * \todo Verify that the exposure and gain applied by the sensor for
> >  	 * this frame match what has been requested. This isn't a hard
> > @@ -424,17 +451,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >  	computeExposure(context, frameContext, yGain, iqMeanGain);
> >  	frameCount_++;
> >
> > -	utils::Duration exposureTime = context.configuration.sensor.lineDuration
> > -				     * frameContext.sensor.exposure;
> > -	metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> > -	metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
> > -
> > -	/* \todo Use VBlank value calculated from each frame exposure. */
> > -	uint32_t vTotal = context.configuration.sensor.size.height
> > -			+ context.configuration.sensor.defVBlank;
> > -	utils::Duration frameDuration = context.configuration.sensor.lineDuration
> > -				      * vTotal;
> > -	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> > +	fillMetadata(context, frameContext, metadata);
> >  }
> >
> >  REGISTER_IPA_ALGORITHM(Agc, "Agc")
> > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > index a228d0c37768..8a22263741b6 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.h
> > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > @@ -44,6 +44,8 @@ private:
> >  	utils::Duration filterExposure(utils::Duration exposureValue);
> >  	double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
> >  	double measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const;
> > +	void fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > +			  ControlList &metadata);
> >
> >  	uint64_t frameCount_;
> >

-- 
Regards,

Laurent Pinchart