[PATCH v4 1/3] libcamera: pipeline: Rename pipelines to a shorter name
Julien Vuillaumier
julien.vuillaumier at nxp.com
Mon May 13 10:55:02 CEST 2024
Hi Kieran,
On 09/05/2024 16:35, Kieran Bingham wrote:
> 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?
I noticed the checkstyle report but did not update as it is the current
implementation and because other macros like
REGISTER_CAMERA_SENSOR_HELPER() and REGISTER_IPA_ALGORITHM() don't use
indentation neither for the static definition line.
For consistency with the other macro definitions, I would suggest
keeping the left alignment but do not mind changing.
Thanks,
Julien
>
>
> 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