[PATCH v4 1/3] libcamera: pipeline: Rename pipelines to a shorter name
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu May 9 16:35:45 CEST 2024
Quoting Kieran Bingham (2024-05-09 09:33:36)
> Quoting Julien Vuillaumier (2024-05-03 15:49:17)
> > The PipelineHandlerFactoryBase class has a name that is
> > propagated to the PipelineHandler instance it creates.
> >
> > In present implementation, this name comes from the
> > REGISTER_PIPELINE_HANDLER registration macro. It corresponds to
> > the stringified name of the PipelineHandler derived class.
> > Therefore, PipelineHandler factories and instances names can be
> > quite long such as "PipelineHandlerRkISP1".
> >
> > A libcamera user may have to explicitly refer to a PipelineHandler
> > name for configuration purpose: one usage of the name can be to
> > define a pipeline handlers match list and their priorities.
> > It is desired, for user convenience, to use a short name to
> > designate a pipeline handler. Reusing the short pipeline names
> > already defined in the meson option files is an existing and
> > consistent way of naming pipelines.
> >
> > This change adds an explicit name parameter to the
> > REGISTER_PIPELINE_HANDLER registration macro. That parameter is
> > used to define the name of a pipeline handler factory, instead of
> > the current pipeline handler class name.
> >
> > Each pipeline registration is updated accordingly. The short name
> > assigned corresponds to the pipeline directory name in the
> > source tree. It is consistent with pipelines names used in meson.
>
> All of the above sounds great.
>
> > Changing the pipeline name has an impact on the IPA modules:
> > each module defines a IPAModuleInfo structure. This structure has
> > a pipelineName member defining the pipeline handler name it shall
> > match with.
> > Therefore, each internal IPA module definition has to be changed
> > to have its IPAModuleInfo pipelineName name updated with the short
> > pipeline handler name.
> >
> > In addition to this pipelineName member, the IPAModuleInfo structure
> > also has a name member, associated to the IPA module name.
> > Having renamed the pipelines to a short name, the pipeline name and
> > the IPA module names of the IPAModuleInfo structure are the same:
> > for in-tree IPA, they correspond to the respective pipeline and IPA
> > subdirectories in the source tree.
> > However the IPA name could be different, for instance with a close
> > source IPA implementation built out-of-tree. Thus, it makes sense to
> > keep the IPA name in that structure, as the 2 definitions may not
> > always be redundant.
>
> This was the first thing I noticed in the code ... why do we duplicate.
>
> But it does sound like it makes sense, particularly if we want to
> register or 'select' distinct IPAs which could be forseen in the future.
> Even to be able to select which IPA to prefer as you have done for the
> Pipeline handlers, so I do think this is worthwhile. There are other
> ways we could handle this, with substring matching, or 'compatible'
> strings ... but it could always be updated on top IMO if we decide to.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> > Signed-off-by: Julien Vuillaumier <julien.vuillaumier at nxp.com>
> > ---
> > Documentation/guides/pipeline-handler.rst | 4 +++-
> > include/libcamera/internal/pipeline_handler.h | 4 ++--
> > src/ipa/ipu3/ipu3.cpp | 2 +-
> > src/ipa/rkisp1/rkisp1.cpp | 2 +-
> > src/ipa/rpi/vc4/vc4.cpp | 2 +-
> > src/ipa/simple/soft_simple.cpp | 2 +-
> > src/ipa/vimc/vimc.cpp | 2 +-
> > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 2 +-
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-
> > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 2 +-
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 2 +-
> > src/libcamera/pipeline/simple/simple.cpp | 2 +-
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-
> > src/libcamera/pipeline/vimc/vimc.cpp | 2 +-
> > src/libcamera/pipeline_handler.cpp | 2 ++
> > test/ipa/ipa_interface_test.cpp | 2 +-
> > test/ipa/ipa_module_test.cpp | 2 +-
> > 18 files changed, 22 insertions(+), 18 deletions(-)
> >
> > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > index 728e9676..7e45cdb8 100644
> > --- a/Documentation/guides/pipeline-handler.rst
> > +++ b/Documentation/guides/pipeline-handler.rst
> > @@ -258,7 +258,7 @@ implementations for the overridden class members.
> > return false;
> > }
> >
> > - REGISTER_PIPELINE_HANDLER(PipelineHandlerVivid)
> > + REGISTER_PIPELINE_HANDLER(PipelineHandlerVivid, "vivid")
> >
> > } /* namespace libcamera */
> >
> > @@ -266,6 +266,8 @@ Note that you must register the ``PipelineHandler`` subclass with the pipeline
> > handler factory using the `REGISTER_PIPELINE_HANDLER`_ macro which
> > registers it and creates a global symbol to reference the class and make it
> > available to try and match devices.
> > +String "vivid" is the name assigned to the pipeline, matching the pipeline
> > +subdirectory name in the source tree.
>
> I would write 'The string "vivid" is the name .... ' but that's minor.
> Could be fixed when applying if desired if everyone else is happy with
> this patch.
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>
>
> >
> > .. _REGISTER_PIPELINE_HANDLER: https://libcamera.org/api-html/pipeline__handler_8h.html
> >
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index c96944f4..cc70f69c 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -140,7 +140,7 @@ public:
> > }
> > };
> >
> > -#define REGISTER_PIPELINE_HANDLER(handler) \
> > -static PipelineHandlerFactory<handler> global_##handler##Factory(#handler);
> > +#define REGISTER_PIPELINE_HANDLER(handler, name) \
> > +static PipelineHandlerFactory<handler> global_##handler##Factory(name);
> >
The only checkstyle failure on this series is here, as clang wants to
indent the static definition line.
I think it's fine keeping it aligned left as it was, but I'm also
tempted to indent it just to silence the checker.
Any preferences from anyone?
https://gitlab.freedesktop.org/camera/libcamera/-/jobs/58532646:
------------------------------------------------------------------------------------------------
2598ba4a94839d63ed1904a0080d6b8285220a5c libcamera: pipeline: Rename pipelines to a shorter name
------------------------------------------------------------------------------------------------
--- include/libcamera/internal/pipeline_handler.h
+++ include/libcamera/internal/pipeline_handler.h
@@ -141,6 +142,6 @@
};
#define REGISTER_PIPELINE_HANDLER(handler, name) \
-static PipelineHandlerFactory<handler> global_##handler##Factory(name);
+ static PipelineHandlerFactory<handler> global_##handler##Factory(name);
} /* namespace libcamera */
--
Kieran
> > } /* namespace libcamera */
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 08ee6eb3..994c0eda 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -672,7 +672,7 @@ extern "C" {
> > const struct IPAModuleInfo ipaModuleInfo = {
> > IPA_MODULE_API_VERSION,
> > 1,
> > - "PipelineHandlerIPU3",
> > + "ipu3",
> > "ipu3",
> > };
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 9dc5f53c..5319c98f 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -458,7 +458,7 @@ extern "C" {
> > const struct IPAModuleInfo ipaModuleInfo = {
> > IPA_MODULE_API_VERSION,
> > 1,
> > - "PipelineHandlerRkISP1",
> > + "rkisp1",
> > "rkisp1",
> > };
> >
> > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp
> > index d2159a51..11316e05 100644
> > --- a/src/ipa/rpi/vc4/vc4.cpp
> > +++ b/src/ipa/rpi/vc4/vc4.cpp
> > @@ -583,7 +583,7 @@ extern "C" {
> > const struct IPAModuleInfo ipaModuleInfo = {
> > IPA_MODULE_API_VERSION,
> > 1,
> > - "PipelineHandlerVc4",
> > + "rpi/vc4",
> > "rpi/vc4",
> > };
> >
> > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> > index b9fb58b5..39c49448 100644
> > --- a/src/ipa/simple/soft_simple.cpp
> > +++ b/src/ipa/simple/soft_simple.cpp
> > @@ -389,7 +389,7 @@ extern "C" {
> > const struct IPAModuleInfo ipaModuleInfo = {
> > IPA_MODULE_API_VERSION,
> > 0,
> > - "SimplePipelineHandler",
> > + "simple",
> > "simple",
> > };
> >
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index 2c255778..1bda772f 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -200,7 +200,7 @@ extern "C" {
> > const struct IPAModuleInfo ipaModuleInfo = {
> > IPA_MODULE_API_VERSION,
> > 0,
> > - "PipelineHandlerVimc",
> > + "vimc",
> > "vimc",
> > };
> >
> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > index 63082cea..f4d755c2 100644
> > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > @@ -1112,6 +1112,6 @@ void PipelineHandlerISI::bufferReady(FrameBuffer *buffer)
> > completeRequest(request);
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerISI)
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerISI, "imx8-isi")
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index fa4bd0bb..d1708d42 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1420,6 +1420,6 @@ void IPU3CameraData::frameStart(uint32_t sequence)
> > *testPatternMode);
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3, "ipu3")
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > index 78343553..44e84bcf 100644
> > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > @@ -1061,6 +1061,6 @@ bool PipelineHandlerMaliC55::match(DeviceEnumerator *enumerator)
> > return true;
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerMaliC55)
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerMaliC55, "mali-c55")
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index abb21968..d8f99135 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1322,6 +1322,6 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
> > data->delayedCtrls_->get(buffer->metadata().sequence));
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, "rkisp1")
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > index 947b1e73..013c6519 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > @@ -1018,6 +1018,6 @@ bool Vc4CameraData::findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&em
> > return true;
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerVc4)
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVc4, "rpi/vc4")
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 61a59926..e62e9172 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -1732,6 +1732,6 @@ void SimplePipelineHandler::releasePipeline(SimpleCameraData *data)
> > }
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)
> > +REGISTER_PIPELINE_HANDLER(SimplePipelineHandler, "simple")
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index ed9c7f88..33464517 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -709,6 +709,6 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer)
> > pipe()->completeRequest(request);
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC, "uvcvideo")
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 5e66ee1d..f681102d 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -623,6 +623,6 @@ void VimcCameraData::paramsBufferReady([[maybe_unused]] unsigned int id,
> > {
> > }
> >
> > -REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)
> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc, "vimc")
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 29e0c98a..0c913e50 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -830,6 +830,8 @@ std::vector<PipelineHandlerFactoryBase *> &PipelineHandlerFactoryBase::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] name Name assigned to the pipeline handler, matching the pipeline
> > + * subdirectory name in the source tree.
> > *
> > * Register a PipelineHandler subclass with the factory and make it available to
> > * try and match devices.
> > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> > index 56f3cd6d..b489b5f6 100644
> > --- a/test/ipa/ipa_interface_test.cpp
> > +++ b/test/ipa/ipa_interface_test.cpp
> > @@ -56,7 +56,7 @@ protected:
> > const std::vector<PipelineHandlerFactoryBase *> &factories =
> > PipelineHandlerFactoryBase::factories();
> > for (const PipelineHandlerFactoryBase *factory : factories) {
> > - if (factory->name() == "PipelineHandlerVimc") {
> > + if (factory->name() == "vimc") {
> > pipe_ = factory->create(nullptr);
> > break;
> > }
> > diff --git a/test/ipa/ipa_module_test.cpp b/test/ipa/ipa_module_test.cpp
> > index bd5e0e4c..ae6f7a52 100644
> > --- a/test/ipa/ipa_module_test.cpp
> > +++ b/test/ipa/ipa_module_test.cpp
> > @@ -57,7 +57,7 @@ protected:
> > const struct IPAModuleInfo testInfo = {
> > IPA_MODULE_API_VERSION,
> > 0,
> > - "PipelineHandlerVimc",
> > + "vimc",
> > "vimc",
> > };
> >
> > --
> > 2.34.1
> >
More information about the libcamera-devel
mailing list