| Message ID | 20201123164319.152742-9-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, 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 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. Thanks j [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++ ? > > Signed-off-by: Kieran Bingham <kieran.bingham@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; > + it = j.find("properties"); > + if (it == j.end()) > + return -ENOENT; > + > + json deviceProperties = *it; > + > + 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); > + > + if (key == "Location") > + properties_.set(properties::Location, value); > + } > + > + return 0; > +} > + > } /* namespace libcamera */ > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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 */
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; + it = j.find("properties"); + if (it == j.end()) + return -ENOENT; + + json deviceProperties = *it; + + 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); + + if (key == "Location") + properties_.set(properties::Location, value); + } + + return 0; +} + } /* namespace libcamera */
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. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/internal/camera_sensor.h | 1 + src/libcamera/camera_sensor.cpp | 50 ++++++++++++++++++++++ 2 files changed, 51 insertions(+)