[libcamera-devel] [RFC PATCH 8/8] libcamera: camera_sensor: Parse configuration properties

[Laurent Pinchart]

Hi Jacopo and Kieran,

On Wed, Nov 25, 2020 at 11:58:30AM +0100, Jacopo Mondi wrote:
> On Mon, Nov 23, 2020 at 04:43:19PM +0000, Kieran Bingham wrote:
> > Search for a camera_sensor.json configuration file specific to libcamera
> > and parse it to see if any properties specific to this CameraSensor are
> > applicable.
> 
> For sake of discussion on the camera sensor database.

I don't think the intent of this patch was to create a sensor database.
That being said, I agree that we shouldn't override camera sensor
properties like done below, I think this patch is more of an example of
how the configuration file parser could be used.

> I don't think this should be a system-wide configuration file but
> rather part of the libcamera sources, with a known path [1] It will
> contain details on the sensor that we cannot retrieve easily from the
> kernel interface, or that are not even exposed by it, even if we
> should keep pushing to get most of those information from a more and
> more expressive v4l2 subdev interface.
> 
> Currently we have information that we cannot retrieve from the kernel
> though. To name a few I can think of
> - Colour Filter Arrangement (even if it could be deduced from the RAW
>   format components sequence)
> - Control delays (see Niklas' delayed controls and RPi's
>   StaggeredControls)
> - Color tuning information and sensitivites (white/black levels ? Gain
>   factors ? Probably we have a V4L2 control for that).
> 
> Ideally, we should be able to push all of them into the kernel, but
> it's a long process. Having a sensor database helps in that regard,
> but makes less pressing the need for decent sensor drivers. One
> example is all the size related information. We -could- get most of
> them through the V4L2 Selection API, but only a few driver actually
> expose them correctly. Recording them in userspace has anyway value,
> so I'm a little debated.
> 
> Also, if we have a 'camera module' to deal with, the sensor database
> (which is not only about sensor anymore) would contain information
> relative to the module integration, like lens information.
> 
> Then there's the part relative to how the sensor itself is integrated
> in the device, and I agree this should be expressed in a system-wide
> configuration file. From there we will have other information we
> cannot retrieve from the subdev interface (mostly because of the lack
> of support in firmware for describing, in example, lens properties, at
> least on device tree). In there we will be able to retreive
> information on the lens mounted on the sensor, where the sensor is
> mounted and its orientation.
> 
> Before seeing the integration in CameraSensor, I would like to discuss
> and see the desired structure of those files. First thing being how to
> identify cameras vs sensor in the two files, if we agree about the
> nature and the intended content of those files.
> 
> [1] being the sensor database a libcamera component, is it worth
> parsing it at run time ? I mean, can we pass it through a script, and
> generate a .h header with all the information easily accessible at
> run-time from C++ ?

I'd rather go that way, either writing it directly in C++, or in a YAML
file.

> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  1 +
> >  src/libcamera/camera_sensor.cpp            | 50 ++++++++++++++++++++++
> >  2 files changed, 51 insertions(+)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index b9eba89f00fb..646f24a966bd 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -69,6 +69,7 @@ protected:
> >
> >  private:
> >  	int generateId();
> > +	int parseConfigurationFile();
> >
> >  	const MediaEntity *entity_;
> >  	std::unique_ptr<V4L2Subdevice> subdev_;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 935de528c496..5a1bda483b9d 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -17,6 +17,7 @@
> >
> >  #include <libcamera/property_ids.h>
> >
> > +#include "libcamera/internal/configuration.h"
> >  #include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/sysfs.h"
> >  #include "libcamera/internal/utils.h"
> > @@ -251,6 +252,12 @@ int CameraSensor::init()
> >  		propertyValue = 0;
> >  	properties_.set(properties::Rotation, propertyValue);
> >
> > +	/*
> > +	 * Properties are pulled from a configuration file on a best effort.
> > +	 * If the file doesn't exist, or has no properties there is no failure.
> > +	 */
> > +	parseConfigurationFile();
> > +
> >  	/* Enumerate, sort and cache media bus codes and sizes. */
> >  	formats_ = subdev_->formats(pad_);
> >  	if (formats_.empty()) {
> > @@ -584,4 +591,47 @@ int CameraSensor::generateId()
> >  	return -EINVAL;
> >  }
> >
> > +int CameraSensor::parseConfigurationFile()
> > +{
> > +	Configuration c;
> > +
> > +	int ret = c.open("camera_sensor.json");
> > +	if (ret) {
> > +		LOG(CameraSensor, Debug) << "No configuration file available";
> > +		return -ENODATA;
> > +	}
> > +
> > +	/* Find properties based on the Camera model_ */
> > +	/* Todo: Spilt parsing out for multiple paths. */
> > +
> > +	/* Find properties based around the Camera Sensor ID */
> > +	json::iterator it = c.data().find(id_);
> > +	if (it == c.data().end())
> > +		return -ENOENT;
> > +
> > +	json j = *it;

This should probably be a const reference ? Thinking about it,
Configuration::data() should likely return a const reference too, to
avoid unintentional modifications.

> > +	it = j.find("properties");
> > +	if (it == j.end())
> > +		return -ENOENT;
> > +
> > +	json deviceProperties = *it;

const reference here too. Unless the json class uses implicit data
sharing internally ?

> > +
> > +	for (auto &[key, value] : deviceProperties.items()) {
> > +		LOG(CameraSensor, Debug) << "Key: " << key << " Value: " << value;
> > +
> > +		if (!value.is_number()) {
> > +			LOG(CameraSensor, Debug) << "Not a number: " << value;
> > +			continue;
> > +		}
> > +
> > +		if (key == "Rotation")
> > +			properties_.set(properties::Rotation, value);

What happens if we attempt to convert the value to an integer when it
contains a different type ?

> > +
> > +		if (key == "Location")
> > +			properties_.set(properties::Location, value);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart