[libcamera-devel] [PATCH 2/8] libcamera: pipeline: add name, major version, and minor version
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 4 11:31:48 CEST 2019
Hi Niklas,
On Tue, Jun 04, 2019 at 12:21:10AM +0200, Niklas Söderlund wrote:
> On 2019-05-27 18:35:34 -0400, Paul Elder wrote:
> > In order to match an IPA module with a pipeline handler, the pipeline
> > handler must have a name and a major and minor version. Add these to
> > PipelineHandler as functions, so that they can be automatically defined
> > by REGISTER_PIPELINE_HANDLER. Also update documentation accordingly.
>
> I'm a bit curious why you went with a major and minor version
> identifiers. I understand they can be useful to describe API version
> (major) and bugfix state (minior).
>
> My question is will this be useful for PipelineHandler <-> IPA matching?
>
> I have really bad experience working with major.minor versioning. In my
> experience it's likely to creep in API changes in minor bug fixes even
> if that is not the intent. Once that happens once, one of two things are
> likely to follow,
>
> 1. On a new release of libcamera all pipeline handlers needs to be
> tested with all minor versions of major versions IPA's not
> shipped with libcamera claim to support.
>
> 2. Begin to exclusively match on a list of known good major.minior
> combinations. Preventing libcamera to work with bugfixes of 3rd
> party IPA who might be well behaved.
>
> Maybe I'm being cynical but I would like for us to consider a single
> integer as a version indicator. This version would describe the API
> between pipeline handler and IPA and not the version of the components
> themself. Or maybe I'm misunderstanding the advantages of major.minor
> design in this problem ;-)
Our problem here is that some IPAs will be shipped as closed-source .so
that we won't be able to update, blocking the update of libcamera. If a
small change is needed in the API between a pipeline handler and its
IPA, in order to enable a new feature, and that change can be done
without breaking backward compatibility, we don't want all the existing
IPAs to stop working.
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> > src/libcamera/include/pipeline_handler.h | 20 ++++++++++++++++++--
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++++-
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 6 +++++-
> > src/libcamera/pipeline/uvcvideo.cpp | 6 +++++-
> > src/libcamera/pipeline/vimc.cpp | 6 +++++-
> > src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++++++++
> > 6 files changed, 58 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index 7da6df1..7963e7a 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -77,6 +77,10 @@ public:
> > bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
> > void completeRequest(Camera *camera, Request *request);
> >
> > + virtual const char *name() = 0;
> > + virtual int majorVersion() = 0;
> > + virtual int minorVersion() = 0;
> > +
> > protected:
> > void registerCamera(std::shared_ptr<Camera> camera,
> > std::unique_ptr<CameraData> data);
> > @@ -112,7 +116,7 @@ private:
> > std::string name_;
> > };
> >
> > -#define REGISTER_PIPELINE_HANDLER(handler) \
> > +#define REGISTER_PIPELINE_HANDLER(handler, majorV, minorV) \
> > class handler##Factory final : public PipelineHandlerFactory \
> > { \
> > public: \
> > @@ -122,7 +126,19 @@ public: \
> > return std::make_shared<handler>(manager); \
> > } \
> > }; \
> > -static handler##Factory global_##handler##Factory;
> > +static handler##Factory global_##handler##Factory; \
> > +const char *handler::name() \
> > +{ \
> > + return #handler; \
> > +} \
> > +int handler::majorVersion() \
> > +{ \
> > + return majorV; \
> > +} \
> > +int handler::minorVersion() \
> > +{ \
> > + return minorV; \
> > +}
> >
> > } /* namespace libcamera */
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 05005c4..a1b06fe 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -212,6 +212,10 @@ public:
> >
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > + const char *name() override;
> > + int majorVersion() override;
> > + int minorVersion() override;
> > +
> > private:
> > IPU3CameraData *cameraData(const Camera *camera)
> > {
> > @@ -1452,6 +1456,6 @@ int CIO2Device::mediaBusToFormat(unsigned int code)
> > }
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3, 0, 1);
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 9b3eea2..7042e7f 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -92,6 +92,10 @@ public:
> >
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > + const char *name() override;
> > + int majorVersion() override;
> > + int minorVersion() override;
> > +
> > private:
> > RkISP1CameraData *cameraData(const Camera *camera)
> > {
> > @@ -499,6 +503,6 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
> > completeRequest(activeCamera_, request);
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, 0, 1);
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 45260f3..27c731e 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -68,6 +68,10 @@ public:
> >
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > + const char *name() override;
> > + int majorVersion() override;
> > + int minorVersion() override;
> > +
> > private:
> > UVCCameraData *cameraData(const Camera *camera)
> > {
> > @@ -263,6 +267,6 @@ void UVCCameraData::bufferReady(Buffer *buffer)
> > pipe_->completeRequest(camera_, request);
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC);
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, 0, 1);
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 0e4eede..d3ff527 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -71,6 +71,10 @@ public:
> >
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > + const char *name() override;
> > + int majorVersion() override;
> > + int minorVersion() override;
> > +
> > private:
> > VimcCameraData *cameraData(const Camera *camera)
> > {
> > @@ -274,6 +278,6 @@ void VimcCameraData::bufferReady(Buffer *buffer)
> > pipe_->completeRequest(camera_, request);
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc);
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, 0, 1);
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index dd56907..8f83307 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -505,6 +505,24 @@ CameraData *PipelineHandler::cameraData(const Camera *camera)
> > * constant for the whole lifetime of the pipeline handler.
> > */
> >
> > +/**
> > + * \fn PipelineHandler::name()
> > + * \brief Retrieve the pipeline handler name
> > + * \return The pipeline handler name
> > + */
> > +
> > +/**
> > + * \fn PipelineHandler::majorVersion()
> > + * \brief Retrieve the pipeline handler major version
> > + * \return The pipeline handler major version
> > + */
> > +
> > +/**
> > + * \fn PipelineHandler::minorVersion()
> > + * \brief Retrieve the pipeline handler minor version
> > + * \return The pipeline handler minor version
> > + */
> > +
> > /**
> > * \class PipelineHandlerFactory
> > * \brief Registration of PipelineHandler classes and creation of instances
> > @@ -586,6 +604,8 @@ std::vector<PipelineHandlerFactory *> &PipelineHandlerFactory::factories()
> > * \def REGISTER_PIPELINE_HANDLER
> > * \brief Register a pipeline handler with the pipeline handler factory
> > * \param[in] handler Class name of PipelineHandler derived class to register
> > + * \param[in] majorV Major version of the PipelineHandler
> > + * \param[in] minorV Minor version of the PipelineHandler
> > *
> > * Register a PipelineHandler subclass with the factory and make it available to
> > * try and match devices.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list