[libcamera-devel] [PATCH v2 2/6] libcamera: ipu3: Register FrameDurations control

[Jacopo Mondi]

Hi Laurent,

On Tue, Feb 09, 2021 at 03:22:15AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Jan 26, 2021 at 06:30:04PM +0100, Jacopo Mondi wrote:
> > Register the FrameDurations control in the IPU3 pipeline handler
> > computed using the vertical blanking limits and the current
> > configuration pixel rate as parameters.
> >
> > The FrameDurations control limits should be updated everytime a new
> > configuration is applied to the sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 35 +++++++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index db0d6b91be70..fe5694f9893a 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -754,6 +754,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >  		return ret;
> >
> >  	ControlInfoMap::Map controls = IPU3Controls;
> > +	const ControlInfoMap &sensorControls = sensor->controls();
> >
> >  	/*
> >  	 * Compute exposure time limits.
> > @@ -766,7 +767,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >  	 */
> >  	double lineDuration = sensorInfo.lineLength
> >  			    / (sensorInfo.pixelRate / 1e6);
> > -	const ControlInfoMap &sensorControls = sensor->controls();
> >  	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> >  	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
> >  	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
> > @@ -781,6 +781,39 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >  	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> >  							defExposure);
> >
> > +	/*
> > +	 * Compute the frame duration limits.
> > +	 *
> > +	 * \todo The frame duration limits depend on the sensor configuration.
> > +	 * Initialize the control using the frame sizes and pixel rate of the
> > +	 * current configuration.
>
> This is the perfect way to make sure it will break in ways that are
> horrible to debug, as the value we report will depend on the
> configuration applied to the subdev the previous time the camera was
> used. Could we use a fixed size instead, maybe sensor->resolution() ?
> We'll still need to make the limits dynamic (and figure out how to
> handle this on the Android side), so a todo comment will still be
> needed, but at least the behaviour will be less random.
>

Please be aware that the exposure time is calculated in the same way.
As controls are initialized at library startup time it is not possible
for libcamera to change the sensor configuration before getting here.

But one could indeed mess with the sensor configuration manually then
run libcamera, and this can effectively cause confusion.

I think I will have to change this in the whole function, and
establish a criteria that applies to all controls. I'll go for sake of
simplicity with the sensor resolution.

> > +	 *
> > +	 * The frame length is computed assuming a fixed line length combined
> > +	 * with the vertical frame sizes.
> > +	 */
> > +	const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
> > +	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> > +	uint32_t lineLength = sensorInfo.outputSize.width + hblank;
> > +
> > +	const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
> > +	std::array<uint32_t, 3> frameHeights{
> > +		(v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height),
> > +		(v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height),
> > +		(v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height),
>
> You can drop the outer parentheses.
>
> > +	};
> > +
> > +	std::vector<int64_t> frameDurations;
> > +	frameDurations.reserve(3);
> > +	std::transform(frameHeights.begin(), frameHeights.end(),
> > +		       std::back_inserter(frameDurations),
> > +		       [&](int32_t frameHeight) {
> > +			       int64_t frameSize = lineLength * frameHeight;
>
> This can be unsigned.
>
> > +			       return frameSize / (sensorInfo.pixelRate / 1000000U);
> > +		       });
>
> Maybe the following would be more readable ?
>
> 	std::array<int64_t, 3> frameDurations;
> 	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
> 		uint64_t frameSize = lineLength * frameHeights[i];
> 		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U)

Possibly, I'll change to the more C version you proposed

Thanks
  j

> 	}
>
> > +	controls[&controls::FrameDurations] = ControlInfo(frameDurations[0],
> > +							  frameDurations[1],
> > +							  frameDurations[2]);
> > +
> >  	/*
> >  	 * Compute the scaler crop limits.
> >  	 *
>
> --
> Regards,
>
> Laurent Pinchart