[libcamera-devel] [RFC 3/7] libcamera: camera_sensor: Factorize out properties
Jacopo Mondi
jacopo at jmondi.org
Mon Jan 20 16:55:55 CET 2020
Hi Niklas,
On Wed, Jan 15, 2020 at 09:03:21PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2019-12-18 15:49:57 +0100, Jacopo Mondi wrote:
> > Factorize CameraSensor properties initialization to a dedicated virtual
>
> Factorize don't seem to match the context :-)
>
> I would here and in the subject s/Factorize/Refactor/
>
> > function that dervied classes could subclass to register sensor-specific
> > properties.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/camera_sensor.cpp | 94 ++++++++++++++++-----------
> > src/libcamera/include/camera_sensor.h | 5 +-
> > 2 files changed, 60 insertions(+), 39 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index d1c9c9bcd58f..9692895d9b7b 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -77,7 +77,7 @@ CameraSensor *CameraSensorFactory::create(const MediaEntity *entity)
> > * Once constructed the instance must be initialized with init().
> > */
> > CameraSensor::CameraSensor(const MediaEntity *entity)
> > - : entity_(entity), properties_(properties::properties)
> > + : properties_(properties::properties), entity_(entity)
> > {
> > subdev_ = new V4L2Subdevice(entity);
> > }
> > @@ -119,42 +119,6 @@ int CameraSensor::init()
> > if (ret < 0)
> > return ret;
> >
> > - /* Retrieve and store the camera sensor properties. */
> > - const ControlInfoMap &controls = subdev_->controls();
> > - int32_t propertyValue;
> > -
> > - /* Camera Location: default is front location. */
> > - const auto &locationControl = controls.find(V4L2_CID_CAMERA_SENSOR_LOCATION);
> > - if (locationControl != controls.end()) {
> > - int32_t v4l2Location =
> > - locationControl->second.def().get<int32_t>();
> > - switch (v4l2Location) {
> > - case V4L2_LOCATION_EXTERNAL:
> > - propertyValue = properties::CameraLocationExternal;
> > - break;
> > - case V4L2_LOCATION_FRONT:
> > - propertyValue = properties::CameraLocationFront;
> > - break;
> > - case V4L2_LOCATION_BACK:
> > - propertyValue = properties::CameraLocationBack;
> > - break;
> > - default:
> > - LOG(CameraSensor, Error)
> > - << "Unsupported camera location: " << v4l2Location;
> > - return -EINVAL;
> > - }
> > - } else {
> > - propertyValue = properties::CameraLocationFront;
> > - }
> > - properties_.set(properties::Location, propertyValue);
> > -
> > - /* Camera Rotation: default is 0 degrees. */
> > - propertyValue = 0;
> > - const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> > - if (rotationControl != controls.end())
> > - propertyValue = rotationControl->second.def().get<int32_t>();
> > - properties_.set(properties::Rotation, propertyValue);
> > -
>
> I Wonder if we should not refactor this into three functions, one
> virtual initProperties() as done already but go even further and add a
> two non-virtual functions initDefaultLocationProperty() and
> initDefaultRotationProperty().
>
> The refactored initProperties() would then call the two new functions
> and allow subclasses to reuse a standard way to parse common
> controls/properties.
>
Would yousplit this up to the point of having one helper per property ?
I'm concerned about code reuse as well, and my idea was that
sub-classes would call the base class CameraSensor::initProperties()
as documented....
> > /* Enumerate and cache media bus codes and sizes. */
> > const ImageFormats formats = subdev_->formats(0);
> > if (formats.isEmpty()) {
> > @@ -185,6 +149,62 @@ int CameraSensor::init()
> > std::sort(mbusCodes_.begin(), mbusCodes_.end());
> > std::sort(sizes_.begin(), sizes_.end());
> >
> > + return initProperties(subdev_->controls());
> > +}
> > +
> > +/**
> > + * \brief Initialize the camera sensor properties
> > + * \param[in] controlMap The map of control information provided by the sensor
> > + *
> > + * This method initializes the camera sensor properties, by inspecting the
> > + * control information reported by the sensor media entity in \a controlMap.
> > + * For each supported standard V4L2 control reported by the sensor, a libcamera
> > + * property is created and registered in the list of properties supported by the
> > + * sensor.
> > + *
> > + * Derived classes are free to override this method to register sensor specific
> > + * properties as they like, by inspecting custom controls or by adding
> > + * properties with pre-defined values, and eventually call this base class
> > + * implementation to register standard ones.
... here to register the default ones. Would you really split it to one per
property ? I would rather instrument the base class initProperty to
register all default ones, except the ones already overridden by the
derived class.
Thanks
j
> > + *
> > + * \return 0 on success, a negative error code otherwise
> > + */
> > +int CameraSensor::initProperties(const ControlInfoMap &controlMap)
> > +{
> > + int32_t propertyValue;
> > +
> > + /* Camera Location: default is front location. */
> > + const auto &locationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_LOCATION);
> > + if (locationControl != controlMap.end()) {
> > + int32_t v4l2Location =
> > + locationControl->second.def().get<int32_t>();
> > + switch (v4l2Location) {
> > + case V4L2_LOCATION_EXTERNAL:
> > + propertyValue = properties::CameraLocationExternal;
> > + break;
> > + case V4L2_LOCATION_FRONT:
> > + propertyValue = properties::CameraLocationFront;
> > + break;
> > + case V4L2_LOCATION_BACK:
> > + propertyValue = properties::CameraLocationBack;
> > + break;
> > + default:
> > + LOG(CameraSensor, Error)
> > + << "Unsupported camera location: " << v4l2Location;
> > + return -EINVAL;
> > + }
> > + } else {
> > + propertyValue = properties::CameraLocationFront;
> > + }
> > + properties_.set(properties::Location, propertyValue);
> > +
> > + /* Camera Rotation: default is 0 degrees. */
> > + propertyValue = 0;
> > + const auto &rotationControl = controlMap.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> > + if (rotationControl != controlMap.end())
> > + propertyValue = rotationControl->second.def().get<int32_t>();
> > + properties_.set(properties::Rotation, propertyValue);
> > +
> > return 0;
> > }
> >
> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> > index 2c09b1bdb61b..43748a6c8535 100644
> > --- a/src/libcamera/include/camera_sensor.h
> > +++ b/src/libcamera/include/camera_sensor.h
> > @@ -38,6 +38,7 @@ public:
> > CameraSensor &operator=(const CameraSensor &) = delete;
> >
> > int init();
> > + virtual int initProperties(const ControlInfoMap &controlMap);
> >
> > const MediaEntity *entity() const { return entity_; }
> > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > @@ -55,6 +56,8 @@ public:
> > const ControlList &properties() const { return properties_; }
> >
> > protected:
> > + ControlList properties_;
> > +
> > friend class CameraSensorFactory;
> > explicit CameraSensor(const MediaEntity *entity);
> > std::string logPrefix() const;
> > @@ -65,8 +68,6 @@ private:
> >
> > std::vector<unsigned int> mbusCodes_;
> > std::vector<Size> sizes_;
> > -
> > - ControlList properties_;
> > };
> >
> > } /* namespace libcamera */
> > --
> > 2.24.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
-------------- 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/20200120/2e19dc7c/attachment.sig>
More information about the libcamera-devel
mailing list