[libcamera-devel] [RFC 3/7] libcamera: camera_sensor: Factorize out properties
Niklas Söderlund
niklas.soderlund at ragnatech.se
Wed Jan 22 15:35:17 CET 2020
Hi Jacopo,
On 2020-01-20 16:55:55 +0100, Jacopo Mondi wrote:
> 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 ?
Maybe or perhaps group them so maybe location and rotation could be
parsed by the same helper.
>
> 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.
I'm not a big fan of the design where the subclass shall overload the
base and then call the base function it overloads. For me it's confusing
to get a good overview of the logic and it can create ambiguity in
implementations. Different subclasses can call the base implementation
first, last or somewhere in-between which can limit what later can be
added to the base. Sometimes the design is unavoidable but is this such
a case?
>
> 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list