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

[Laurent Pinchart]

Hi Jacopo,

On Sat, Mar 09, 2019 at 09:39:49PM +0100, Jacopo Mondi wrote:
> On Sat, Mar 02, 2019 at 11:23:52PM +0200, Laurent Pinchart wrote:
> > 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.

Retrieving the size at open() time and caching it would be a good idea I
think, but that can be done outside of this patch if you prefer (please
add a \todo in that case).

> >>  	}
> >>
> >> -	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?

I think that's a good idea, it will allow us to standardize comparisons
in the library.

> >> +
> >> +		++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.
> 
> >>
> >>  	return configs;
> >>  }
> >

-- 
Regards,

Laurent Pinchart