[libcamera-devel] (no subject)

Message ID	E1n5wCX-0004MQ-3t@mail-02.1984.is
State	New
Headers	show Return-Path: <libcamera-devel-bounces@lists.libcamera.org> Message-Id: <E1n5wCX-0004MQ-3t@mail-02.1984.is> From: victor@westerhu.is Date: Fri, 07 Jan 2022 20:51:05 +0000 Subject: [libcamera-devel] (no subject) Precedence: list Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" <libcamera-devel-bounces@lists.libcamera.org>
Series	[libcamera-devel] (no subject)
Related	show [libcamera-devel] (no subject)

Victor Westerhuis Jan. 7, 2022, 8:51 p.m. UTC

From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001
From: Victor Westerhuis <victor@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@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?

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(-)

Kieran Bingham Jan. 8, 2022, 9:47 p.m. UTC | #1

Hi Victor,

Quoting victor@westerhu.is (2022-01-07 20:51:05)
> From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001
> From: Victor Westerhuis <victor@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@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.

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>
>  
> -- 
> 2.34.1
>

Laurent Pinchart Jan. 8, 2022, 10:19 p.m. UTC | #2

Hello,

On Sat, Jan 08, 2022 at 09:47:36PM +0000, Kieran Bingham wrote:
> Quoting victor@westerhu.is (2022-01-07 20:51:05)
> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001
> > From: Victor Westerhuis <victor@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@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.

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>
> >

Victor Westerhuis Jan. 9, 2022, 12:52 a.m. UTC | #3

Laurent Pinchart schreef op 08.01.2022 23:19:
> Hello,
> 
> On Sat, Jan 08, 2022 at 09:47:36PM +0000, Kieran Bingham wrote:
>> Quoting victor@westerhu.is (2022-01-07 20:51:05)
>> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001
>> > From: Victor Westerhuis <victor@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@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.
> 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>
>> >

Laurent Pinchart Jan. 9, 2022, 2:32 a.m. UTC | #4

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@westerhu.is (2022-01-07 20:51:05)
> >> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001
> >> > From: Victor Westerhuis <victor@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@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@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@westerhu.is>
Signed-off-by: Laurent Pinchart <laurent.pinchart@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{}) },
+		{ &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>
> >> >

Kieran Bingham Jan. 10, 2022, 9:37 a.m. UTC | #5

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@westerhu.is (2022-01-07 20:51:05)
> > >> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001
> > >> > From: Victor Westerhuis <victor@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@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@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@westerhu.is>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@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

Naushir Patuck Jan. 10, 2022, 10:10 a.m. UTC | #6

Hi everyone,

Victor, thanks for the patch!

On Mon, 10 Jan 2022 at 09:37, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> 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@westerhu.is (2022-01-07 20:51:05)
> > > >> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00
> 2001
> > > >> > From: Victor Westerhuis <victor@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@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@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@westerhu.is>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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?
>

Scaler crop is to only control in this list that does not really belong in
the IPA. Perhaps
there needs to be 2 lists of controls, one for the IPA and one for the
pipeline handler?
Not the biggest fan of this approach :(

If one list is acceptable, I am happy to remove it from the header and keep
in a separate
cpp file.

Thanks,
Naush



>
> --
> 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
>

Laurent Pinchart Jan. 10, 2022, 11:08 a.m. UTC | #7

Hi Naush, everyone,

On Mon, Jan 10, 2022 at 10:10:00AM +0000, Naushir Patuck wrote:
> On Mon, 10 Jan 2022 at 09:37, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2022-01-09 02:32:25)
> > > 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@westerhu.is (2022-01-07 20:51:05)
> > > > >> > From 4f419bd0310462616a107c89510a4864a3b8db31 Mon Sep 17 00:00:00 2001
> > > > >> > From: Victor Westerhuis <victor@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@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@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@westerhu.is>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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?

Good point.

> Scaler crop is to only control in this list that does not really belong in the IPA. Perhaps
> there needs to be 2 lists of controls, one for the IPA and one for the pipeline handler?
> Not the biggest fan of this approach :(

I think we should have two indeed. In the general case, the pipeline
handler can control processing steps that the IPA isn't aware of, so it
would add controls to the ControlInfoMap, merging its own controls with
the one provided by the IPA.

Does anyone want to give it a try in a v2 of this patch ? :-)

> If one list is acceptable, I am happy to remove it from the header and keep in a separate
> cpp file.
> 
> > > +               { &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>
> > > > >> >

[libcamera-devel] (no subject)
diff mbox series

Commit Message

Comments

Patch
diff mbox series

[libcamera-devel] (no subject) 15261 diff mbox series

Commit Message

Comments

Patch 15261 diff mbox series

[libcamera-devel] (no subject)
diff mbox series

Patch
diff mbox series