[libcamera-devel] [PATCH 02/10] libcamera: ipu3: Get default image sizes from sensor

[Jacopo Mondi]

Hi Laurent,

On Sat, Mar 02, 2019 at 11:23:52PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Feb 28, 2019 at 09:04:02PM +0100, Jacopo Mondi wrote:
> > Inspect all image sizes provided by the sensor and select the
> > biggest one to be returned as default stream configuration instead of
> > returning the currently applied one.
> >
> > Hardcode the stream pixel format to the one produced by the CIO2 unit,
> > to be changed to the one provided by the ImgU.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 44 ++++++++++++++++++++--------
> >  1 file changed, 32 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index d3f1d9a95f81..4f1ab72debf8 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -71,6 +71,8 @@ public:
> >  	bool match(DeviceEnumerator *enumerator);
> >
> >  private:
> > +	static constexpr unsigned int IPU3_BUF_NUM = 4;
>
> IPU3_BUF_COUNT or IPU3_BUFFER_COUNT to match the name of the bufferCount
> field ?

I'll use IPU3_BUFFER_COUNT
>
> > +
> >  	IPU3CameraData *cameraData(const Camera *camera)
> >  	{
> >  		return static_cast<IPU3CameraData *>(
> > @@ -102,27 +104,45 @@ std::map<Stream *, StreamConfiguration>
> >  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> >  					 std::vector<Stream *> &streams)
> >  {
> > +	std::map<unsigned int, std::vector<SizeRange>> formats;
>
> How about movinf this line down to declare and initialize the variable
> at the same time ?
>

Will do.

> >  	std::map<Stream *, StreamConfiguration> configs;
> >  	IPU3CameraData *data = cameraData(camera);
> >  	V4L2Subdevice *sensor = data->cio2.sensor;
> > -	V4L2SubdeviceFormat format = {};
> > +	StreamConfiguration *config = &configs[&data->stream_];
> > +
> > +	config->pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
> > +	config->bufferCount = IPU3_BUF_NUM;
> >
> >  	/*
> > -	 * FIXME: As of now, return the image format reported by the sensor.
> > -	 * In future good defaults should be provided for each stream.
> > +	 * Use the largest image size the sensor provides or
> > +	 * use a default one.
> >  	 */
> > -	if (sensor->getFormat(0, &format)) {
> > -		LOG(IPU3, Error) << "Failed to create stream configurations";
> > -		return configs;
> > +	formats = sensor->formats(0);
> > +	if (formats.empty()) {
> > +		config->width = 1920;
> > +		config->height = 1080;
> > +		LOG(IPU3, Info)
> > +			<< "Use default stream sizes " << config->width
> > +			<< "x" << config->height;
>
> It doesn't make much sense to fall back to a default value here, all
> sensor drivers must report the sizes they support. If you're worried
> they may not, let's add a check when creating the camera and error out.
> You could then cache the formats supported by the sensor in the
> CIO2Device structure.
>

Yes, I agree, defaults are bad. I'll drop this part and assume the
sensor provides a list of formats.

> >  	}
> >
> > -	StreamConfiguration config = {};
> > -	config.width = format.width;
> > -	config.height = format.height;
> > -	config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
> > -	config.bufferCount = 4;
> > +	auto it = formats.begin();
> > +	while (it != formats.end()) {
>
> Why do you dislike for loops ? :-)
>
> 	for (auto it = formats.begin(); it != formats.end(); ++it) {
>
> And I'd even make it a const auto it and a const SizeRange & below as
> you don't modify it.

I don't know, I always associate iterators to while loops. Bad habit,
I'll change this.

>
> > +		for (SizeRange &range : it->second) {
> > +			if (range.maxWidth <= config->width ||
> > +			    range.maxHeight <= config->height)
> > +				continue;
> > +
> > +			config->width = range.maxWidth;
> > +			config->height = range.maxHeight;
> > +		}
>
> What happens if the sensor supports two sizes with different aspect
> ratios, let's say 4:3 and 16:9, where one has a smaller width by larger
> height ? Shouldn't we compare the areas instead of the individual
> dimensions ?
>

Should I add an area comparator to Geometry maybe?

> > +
> > +		++it;
> > +	}
> >
> > -	configs[&data->stream_] = config;
> > +	LOG(IPU3, Info) << "Stream format set to = (" << config->width << "x"
> > +			<< config->height << ") - 0x" << std::hex
>
> I know how much you dislike the streams API, but 0x%08x would be nice.
>
> > +			<< config->pixelFormat;
>
> Wouldn't Debug make more sense than Info here ?

As we agreed offline, most of the Info printouts in this series will
be turned into Debug ones.

Thanks
  j

>
> >
> >  	return configs;
> >  }
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190309/63b0db13/attachment.sig>