[libcamera-devel] Fix build with LTO enabled
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jan 10 10:37:50 CET 2022
Hi Laurent,
Quoting Laurent Pinchart (2022-01-09 02:32:25)
> Hi Victor,
>
> On Sun, Jan 09, 2022 at 01:52:53AM +0100, Victor Westerhuis wrote:
> > Laurent Pinchart schreef op 08.01.2022 23:19:
> > > On Sat, Jan 08, 2022 at 09:47:36PM +0000, Kieran Bingham wrote:
> > >> Quoting victor at westerhu.is (2022-01-07 20:51:05)
> > >> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001
> > >> > From: Victor Westerhuis <victor at westerhu.is>
> > >> > Date: Fri, 7 Jan 2022 17:10:53 +0100
> > >> > Subject: [PATCH] Fix build with LTO enabled
> > >> >
> > >> > libcamera::RPi::Controls in raspberrypi.h depends on
> > >> > libcamera::controls::controls in control_ids.cpp, instantiated
> > >> > from control_ids.cpp.in.
> > >> >
> > >> > The order of initialization is not defined between these two
> > >> > namespace scope objects. This patch changes RPi::Controls to a
> > >> > function-level static, initialized on first use and therefore
> > >> > safe to use.
> > >> >
> > >> > This leads to warnings about getControls not being used in
> > >> > src/ipa/raspberrypi/raspberrypi.cpp and
> > >> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > >> > (through src/libcamera/pipeline/raspberrypi/rpi_stream.h).
> > >> > This patch therefore drops the include without ill effects.
> > >> >
> > >> > Signed-off-by: Victor Westerhuis <victor at westerhu.is>
> > >> > ---
> > >> > This patch fixes https://bugs.libcamera.org/show_bug.cgi?id=83
> > >> >
> > >> > Perhaps this data should not be in a header at all?
> > >>
> > >> Looking at this patch, it certainly looks like this should be moved to
> > >> the .cpp file, and doesn't need to be in the .h file.
> > >
> > > The reason why the ControlInfoMap is defined in the header file is that
> > > it's used by the IPA module and the pipeline handler. If you moved it to
> > > a .cpp file on one side, it wouldn't be accessible to the other side.
> >
> > While writing my patch, I noticed that the data in the header is not
> > currently referenced elsewhere. That's why after changing the variable
> > to a getter function I had to remove the include from two other files,
> > to prevent a warning about unused functions.
>
> That's a good point, but conceptually, this is data that is part of the
> IPA module, and is used by the pipeline handler. We could move it to the
> pipeline handler implementation, but it means it would need to change
> when the IPA changes, which isn't right. Storing it in the IPA module
> and passing it to the pipeline handler at init time would be better.
>
> How about the following (untested) patch ?
>
> From: Victor Westerhuis <victor at westerhu.is>
> Date: Sun, 9 Jan 2022 04:27:51 +0200
> Subject: [PATCH] ipa: raspberrypi: Fix build with LTO enabled
>
> libcamera::RPi::Controls in raspberrypi.h depends on
> libcamera::controls::controls in control_ids.cpp, instantiated from
> control_ids.cpp.in.
>
> The order of initialization is not defined between these two namespace
> scope objects, resulting in a runtime failure when compiling with LTO
> enabled.
>
> To solve this, address the long-standing issue of the ControlInfoMap
> being defined in a shared header, by moving it to the Raspberry Pi IPA
> and passing it to the pipeline handler through the IPA init() function.
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=83
> Signed-off-by: Victor Westerhuis <victor at westerhu.is>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/ipa/raspberrypi.h | 55 -------------------
> include/libcamera/ipa/raspberrypi.mojom | 3 +-
> src/ipa/raspberrypi/raspberrypi.cpp | 39 ++++++++++++-
> .../pipeline/raspberrypi/raspberrypi.cpp | 15 +++--
> .../pipeline/raspberrypi/rpi_stream.h | 1 -
> 5 files changed, 47 insertions(+), 66 deletions(-)
> delete mode 100644 include/libcamera/ipa/raspberrypi.h
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> deleted file mode 100644
> index 7f705e49411d..000000000000
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ /dev/null
> @@ -1,55 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2019-2020, Raspberry Pi Ltd.
> - *
> - * raspberrypi.h - Image Processing Algorithm interface for Raspberry Pi
> - */
> -
> -#pragma once
> -
> -#include <stdint.h>
> -
> -#include <libcamera/control_ids.h>
> -#include <libcamera/controls.h>
> -
> -#ifndef __DOXYGEN__
> -
> -namespace libcamera {
> -
> -namespace RPi {
> -
> -/*
> - * List of controls handled by the Raspberry Pi IPA
> - *
> - * \todo This list will need to be built dynamically from the control
> - * algorithms loaded by the json file, once this is supported. At that
> - * point applications should check first whether a control is supported,
> - * and the pipeline handler may be reverted so that it aborts when an
> - * unsupported control is encountered.
> - */
> -static const ControlInfoMap Controls({
> - { &controls::AeEnable, ControlInfo(false, true) },
> - { &controls::ExposureTime, ControlInfo(0, 999999) },
> - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> - { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> - { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> - { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> - { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> - { &controls::AwbEnable, ControlInfo(false, true) },
> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> - { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> - { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> - { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> - { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> - { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> - { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> - { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> - }, controls::controls);
> -
> -} /* namespace RPi */
> -
> -} /* namespace libcamera */
> -
> -#endif /* __DOXYGEN__ */
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index acd3cafe6c91..275cf946ee9a 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -45,7 +45,8 @@ struct StartConfig {
>
> interface IPARPiInterface {
> init(libcamera.IPASettings settings)
> - => (int32 ret, SensorConfig sensorConfig);
> + => (int32 ret, SensorConfig sensorConfig,
> + libcamera.ControlInfoMap controls);
> start(libcamera.ControlList controls) => (StartConfig startConfig);
> stop();
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 677126a3a2b3..e919d91e4c33 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -24,7 +24,6 @@
> #include <libcamera/framebuffer.h>
> #include <libcamera/ipa/ipa_interface.h>
> #include <libcamera/ipa/ipa_module_info.h>
> -#include <libcamera/ipa/raspberrypi.h>
> #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> #include <libcamera/request.h>
>
> @@ -89,7 +88,8 @@ public:
> munmap(lsTable_, ipa::RPi::MaxLsGridSize);
> }
>
> - int init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig) override;
> + int init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig,
> + ControlInfoMap *ipaControls) override;
> void start(const ControlList &controls, ipa::RPi::StartConfig *startConfig) override;
> void stop() override {}
>
> @@ -175,8 +175,39 @@ private:
> Duration maxFrameDuration_;
> };
>
> -int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig)
> +int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConfig,
> + ControlInfoMap *ipaControls)
> {
> + /*
> + * List of controls handled by the Raspberry Pi IPA
> + *
> + * \todo This list will need to be built dynamically from the control
> + * algorithms loaded by the json file, once this is supported. At that
> + * point applications should check first whether a control is supported,
> + * and the pipeline handler may be reverted so that it aborts when an
> + * unsupported control is encountered.
> + */
> + static const ControlInfoMap rpiControls({
> + { &controls::AeEnable, ControlInfo(false, true) },
> + { &controls::ExposureTime, ControlInfo(0, 999999) },
> + { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> + { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> + { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> + { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> + { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> + { &controls::AwbEnable, ControlInfo(false, true) },
> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> + { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> + { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> + { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> + { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> + { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
Is the ScalerCrop an IPA related control?
--
Kieran
> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> + { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> + }, controls::controls);
> +
> /*
> * Load the "helper" for this sensor. This tells us all the device specific stuff
> * that the kernel driver doesn't. We only do this the first time; we don't need
> @@ -206,6 +237,8 @@ int IPARPi::init(const IPASettings &settings, ipa::RPi::SensorConfig *sensorConf
> controller_.Read(settings.configurationFile.c_str());
> controller_.Initialise();
>
> + *ipaControls = rpiControls;
> +
> return 0;
> }
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 49d7ff23209f..c9bffd647068 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -20,7 +20,6 @@
> #include <libcamera/camera.h>
> #include <libcamera/control_ids.h>
> #include <libcamera/formats.h>
> -#include <libcamera/ipa/raspberrypi.h>
> #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> #include <libcamera/ipa/raspberrypi_ipa_proxy.h>
> #include <libcamera/logging.h>
> @@ -191,7 +190,8 @@ public:
>
> void frameStarted(uint32_t sequence);
>
> - int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> + int loadIPA(ipa::RPi::SensorConfig *sensorConfig,
> + ControlInfoMap *ipaControls);
> int configureIPA(const CameraConfiguration *config);
>
> void enumerateVideoDevices(MediaLink *link);
> @@ -1199,7 +1199,9 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> data->sensorFormats_ = populateSensorFormats(data->sensor_);
>
> ipa::RPi::SensorConfig sensorConfig;
> - if (data->loadIPA(&sensorConfig)) {
> + ControlInfoMap ipaControls;
> +
> + if (data->loadIPA(&sensorConfig, &ipaControls)) {
> LOG(RPI, Error) << "Failed to load a suitable IPA library";
> return -EINVAL;
> }
> @@ -1250,7 +1252,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> data->sensorMetadata_ = sensorConfig.sensorMetadata;
>
> /* Register the controls that the Raspberry Pi IPA can handle. */
> - data->controlInfo_ = RPi::Controls;
> + data->controlInfo_ = ipaControls;
> /* Initialize the camera properties. */
> data->properties_ = data->sensor_->properties();
>
> @@ -1467,7 +1469,8 @@ void RPiCameraData::frameStarted(uint32_t sequence)
> delayedCtrls_->applyControls(sequence);
> }
>
> -int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> +int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig,
> + ControlInfoMap *ipaControls)
> {
> ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);
>
> @@ -1493,7 +1496,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
>
> IPASettings settings(configurationFile, sensor_->model());
>
> - return ipa_->init(settings, sensorConfig);
> + return ipa_->init(settings, sensorConfig, ipaControls);
> }
>
> int RPiCameraData::configureIPA(const CameraConfiguration *config)
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index d6f49d34f8c8..b0fc1119aabb 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -12,7 +12,6 @@
> #include <unordered_map>
> #include <vector>
>
> -#include <libcamera/ipa/raspberrypi.h>
> #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> #include <libcamera/stream.h>
>
> > > I think the information should be store din the IPA module, and
> > > communicated to the pipeline handler at init time.
> > >
> > >> Naush, what do you think?
> > >>
> > >> Maybe this set of controls should be part of the class too. Would that
> > >> help guarantee the order of construction?
> > >>
> > >> > Please CC me when responding, since I'm not subscribed to this mailing
> > >> > list.
> > >> >
> > >> > include/libcamera/ipa/raspberrypi.h | 42 ++++++++++---------
> > >> > src/ipa/raspberrypi/raspberrypi.cpp | 1 -
> > >> > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-
> > >> > .../pipeline/raspberrypi/rpi_stream.h | 1 -
> > >> > 4 files changed, 24 insertions(+), 22 deletions(-)
> > >> >
> > >> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > >> > index 7f705e49..545df355 100644
> > >> > --- a/include/libcamera/ipa/raspberrypi.h
> > >> > +++ b/include/libcamera/ipa/raspberrypi.h
> > >> > @@ -27,26 +27,30 @@ namespace RPi {
> > >> > * and the pipeline handler may be reverted so that it aborts when an
> > >> > * unsupported control is encountered.
> > >> > */
> > >> > -static const ControlInfoMap Controls({
> > >> > - { &controls::AeEnable, ControlInfo(false, true) },
> > >> > - { &controls::ExposureTime, ControlInfo(0, 999999) },
> > >> > - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > >> > - { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> > >> > - { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > >> > - { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> > >> > - { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> > >> > - { &controls::AwbEnable, ControlInfo(false, true) },
> > >> > - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > >> > - { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> > >> > - { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> > >> > - { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> > >> > - { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> > >> > - { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> > >> > - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> > >> > - { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> > >> > - { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> > >> > - { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> > >> > +static const ControlInfoMap &getControls()
> > >> > +{
> > >> > + static const ControlInfoMap controls({
> > >> > + { &controls::AeEnable, ControlInfo(false, true) },
> > >> > + { &controls::ExposureTime, ControlInfo(0, 999999) },
> > >> > + { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > >> > + { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> > >> > + { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > >> > + { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> > >> > + { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> > >> > + { &controls::AwbEnable, ControlInfo(false, true) },
> > >> > + { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > >> > + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> > >> > + { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> > >> > + { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> > >> > + { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> > >> > + { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> > >> > + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> > >> > + { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> > >> > + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> > >> > + { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> > >> > }, controls::controls);
> > >> > + return controls;
> > >> > +}
> > >> >
> > >> > } /* namespace RPi */
> > >> >
> > >> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > index 0ed41385..b2717dfc 100644
> > >> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > >> > @@ -24,7 +24,6 @@
> > >> > #include <libcamera/framebuffer.h>
> > >> > #include <libcamera/ipa/ipa_interface.h>
> > >> > #include <libcamera/ipa/ipa_module_info.h>
> > >> > -#include <libcamera/ipa/raspberrypi.h>
> > >> > #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> > >> > #include <libcamera/request.h>
> > >> >
> > >> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > index 168bbcef..a48f1130 100644
> > >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >> > @@ -1244,7 +1244,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> > >> > data->sensorMetadata_ = sensorConfig.sensorMetadata;
> > >> >
> > >> > /* Register the controls that the Raspberry Pi IPA can handle. */
> > >> > - data->controlInfo_ = RPi::Controls;
> > >> > + data->controlInfo_ = RPi::getControls();
> > >> > /* Initialize the camera properties. */
> > >> > data->properties_ = data->sensor_->properties();
> > >> >
> > >> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > >> > index d6f49d34..b0fc1119 100644
> > >> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > >> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > >> > @@ -12,7 +12,6 @@
> > >> > #include <unordered_map>
> > >> > #include <vector>
> > >> >
> > >> > -#include <libcamera/ipa/raspberrypi.h>
> > >> > #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> > >> > #include <libcamera/stream.h>
> > >> >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list