[PATCH v4 1/3] libcamera: pipeline: Rename pipelines to a shorter name
Julien Vuillaumier
julien.vuillaumier at nxp.com
Mon May 13 10:29:05 CEST 2024
Hi Kieran,
On 09/05/2024 10:33, Kieran Bingham wrote:
> 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.
Agreed, having a way to select a specific IPA module could be a
convenient addition for the future. As of now, there is presumably a way
to do so by setting the LIBCAMERA_IPA_MODULE_PATH variable. But it
requires the IPA modules to be installed in different paths, which is
not always desired.
>
> 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.
>
Point noted - I will add that fix if there is a new version of the patch
set then.
Thanks,
Julien
>
> 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);
>>
>> } /* 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