[libcamera-devel] [PATCH 03/12] libcamera: ipu3: Register Exposure control

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jan 18, 2021 at 01:27:32PM +0100, Jacopo Mondi wrote:
> On Mon, Jan 11, 2021 at 12:22:54AM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > > +	 */
> > > +	float pixelRate = sensorInfo.pixelRate / 1e6f;
> > > +	const ControlInfoMap &sensorControls = sensor->controls();
> > > +	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> > > +	int32_t minExposure = v4l2Exposure.min().get<int32_t>()
> > > +			    * sensorInfo.lineLength / pixelRate;
> > > +	int32_t maxExposure = v4l2Exposure.max().get<int32_t>()
> > > +			    * sensorInfo.lineLength / pixelRate;
> > > +	int32_t defExposure = v4l2Exposure.def().get<int32_t>()
> > > +			    * sensorInfo.lineLength / pixelRate;
> >
> > For long exposure time this will overflow as the first two operands are
> > 32-bit integers. How about the following ?
> >
> > 	double lineDuration = sensorInfo.pixelRate / 1e6f /pixelRate;
> 
> did you mean:
>         sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6f)
> or rather
>         sensorInfo.lineLength * 1e6f / sensorInfo.pixelRate

Yes, that's what I meant, sorry.

> but there's an overflow risk as well here.

In your code v4l2Exposure.min().get<int32_t>() is an int32_t, and
sensorInfo.lineLength is an uint32_t. Multiplying the two has a high
risk of overflow. Computing lineDuration as 

	double lineDuration = sensorInfo.lineLength * 1e6f
			    / sensorInfo.pixelRate;

won't overflow as multiplying by 1e6f first will produce a float value.

> > 	...
> > 	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;

Then these multiplications may overflow only if the result doesn't fit
in an int32_t, which would happen if the exposure time is larger than
~35 minutes. I think that's unlikely.

> > > +
> > > +	/*
> > > +	 * \todo Report the actual exposure time, use the default for the
> > > +	 * moment.
> > > +	 */
> > > +	data->exposureTime_ = defExposure;
> > > +
> > > +	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
> > > +							defExposure);
> > > +
> > > +	data->controlInfo_ = std::move(controls);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /**
> > >   * \brief Initialise ImgU and CIO2 devices associated with cameras
> > >   *
> > > @@ -776,8 +833,9 @@ int PipelineHandlerIPU3::registerCameras()
> > >  		/* Initialize the camera properties. */
> > >  		data->properties_ = cio2->sensor()->properties();
> > >
> > > -		/* Initialze the camera controls. */
> > > -		data->controlInfo_ = IPU3Controls;
> > > +		ret = initControls(data.get());
> > > +		if (ret)
> > > +			continue;
> > >
> > >  		/**
> > >  		 * \todo Dynamically assign ImgU and output devices to each
> > > @@ -842,6 +900,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
> > >
> > >  	/* Mark the request as complete. */
> > >  	request->metadata().set(controls::draft::PipelineDepth, 3);
> >
> > 	/* \todo Move the ExposureTime control to the IPA */
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > +	request->metadata().set(controls::ExposureTime, exposureTime_);
> > >  	pipe_->completeRequest(request);
> > >  }

-- 
Regards,

Laurent Pinchart