[libcamera-devel] [PATCH 2/8] libcamera: pipeline: add name, major version, and minor version
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Jun 4 00:21:10 CEST 2019
Hi Paul,
Thanks for your work.
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 ;-)
>
> 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.
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list