[libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce IpaBase class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Apr 27 17:54:15 CEST 2023
Hi Naush,
Thank you for the patch.
On Wed, Apr 26, 2023 at 02:10:52PM +0100, Naushir Patuck via libcamera-devel wrote:
> Create a new IpaBase class that handles general purpose housekeeping
> duties for the Raspberry Pi IPA. Code the implementation of new class is
Did you mean s/Code the implementation/The implementation/ and
s/of new class/of the new class/ ?
> essentially pulled from the existing ipa/rpi/vc4/raspberrypi.cpp
> file with a minimal amount of refactoring.
>
> Create a derived IpaVc4 class from IpaBase that handles the VC4 pipeline
> specific tasks of the IPA. Again, code for this class implementation is
> taken from the existing ipa/rpi/vc4/raspberrypi.cpp with a
> minimal amount of refactoring.
>
> The goal of this change is to allow third parties to implement their own
> IPA running on the Raspberry Pi without duplicating all of the IPA
> housekeeping tasks.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> src/ipa/meson.build | 5 +-
> .../raspberrypi.cpp => common/ipa_base.cpp} | 1219 +++++------------
> src/ipa/rpi/common/ipa_base.h | 125 ++
> src/ipa/rpi/common/meson.build | 7 +
> src/ipa/rpi/vc4/meson.build | 8 +-
> src/ipa/rpi/vc4/vc4.cpp | 540 ++++++++
> 6 files changed, 1012 insertions(+), 892 deletions(-)
> rename src/ipa/rpi/{vc4/raspberrypi.cpp => common/ipa_base.cpp} (65%)
> create mode 100644 src/ipa/rpi/common/ipa_base.h
> create mode 100644 src/ipa/rpi/common/meson.build
> create mode 100644 src/ipa/rpi/vc4/vc4.cpp
>
> diff --git a/src/ipa/meson.build b/src/ipa/meson.build
> index 10d3b44ca7b6..0c622c38a4a0 100644
> --- a/src/ipa/meson.build
> +++ b/src/ipa/meson.build
> @@ -37,13 +37,14 @@ endif
>
> enabled_ipa_modules = []
>
> -# If the Raspberry Pi VC4 IPA is enabled, ensure we include the rpi/cam_helper
> -# and rpi/controller subdirectories in the build.
> +# If the Raspberry Pi VC4 IPA is enabled, ensure we include the cam_helper,
> +# common and controller subdirectories in the build.
> #
> # This is done here and not within rpi/vc4/meson.build as meson does not
> # allow the subdir command to traverse up the directory tree.
> if pipelines.contains('rpi/vc4')
> subdir('rpi/cam_helper')
> + subdir('rpi/common')
> subdir('rpi/controller')
> endif
>
> diff --git a/src/ipa/rpi/vc4/raspberrypi.cpp b/src/ipa/rpi/common/ipa_base.cpp
> similarity index 65%
> rename from src/ipa/rpi/vc4/raspberrypi.cpp
> rename to src/ipa/rpi/common/ipa_base.cpp
> index e3ca8e2f7cbe..becada5973ad 100644
> --- a/src/ipa/rpi/vc4/raspberrypi.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -1,60 +1,30 @@
[snip]
> @@ -62,8 +32,7 @@ namespace libcamera {
> using namespace std::literals::chrono_literals;
> using utils::Duration;
>
> -/* Number of metadata objects available in the context list. */
> -constexpr unsigned int numMetadataContexts = 16;
> +namespace {
>
> /* Number of frame length times to hold in the queue. */
> constexpr unsigned int FrameLengthsQueueSize = 10;
> @@ -80,7 +49,7 @@ constexpr Duration defaultMaxFrameDuration = 250.0s;
> * we rate-limit the controller Prepare() and Process() calls to lower than or
> * equal to this rate.
> */
> -constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
> +static constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
Everything contained in an anonymous namespace becomes static. You can
drop the static keyword from other variables.
>
> /* List of controls handled by the Raspberry Pi IPA */
> static const ControlInfoMap::Map ipaControls{
> @@ -116,118 +85,23 @@ static const ControlInfoMap::Map ipaAfControls{
> { &controls::LensPosition, ControlInfo(0.0f, 32.0f, 1.0f) }
> };
>
> +} /* namespace */
> +
[snip]
> diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
> new file mode 100644
> index 000000000000..b9fbf9414d63
> --- /dev/null
> +++ b/src/ipa/rpi/common/ipa_base.h
> @@ -0,0 +1,125 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2023, Raspberry Pi Ltd
> + *
> + * ipa_base.cpp - Raspberry Pi IPA base class
ipa_base.h
> + */
> +#pragma once
> +
> +#include <array>
> +#include <deque>
> +#include <vector>
You need <map> here, but can drop <vector> as it's only used to
implement the interface of the base class, and so is provided by
libcamera/ipa/ipa_interface.h. You also need stdint.h.
> +
#include <libcamera/base/span.h>
> +#include <libcamera/base/utils.h>
> +
> +#include <libcamera/controls.h>
> +
> +#include <libcamera/ipa/ipa_interface.h>
> +#include <libcamera/ipa/ipa_module_info.h>
Not needed.
> +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> +
> +#include "libcamera/internal/mapped_framebuffer.h"
> +
> +#include "cam_helper/cam_helper.h"
> +#include "controller/agc_status.h"
> +#include "controller/camera_mode.h"
> +#include "controller/controller.h"
> +#include "controller/metadata.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::RPi {
> +
> +class IpaBase : public IPARPiInterface
> +{
> +public:
> + IpaBase();
> + virtual ~IpaBase() = 0;
That's a bit weird, why do you need a virtual abstract destructor ?
> +
> + int32_t init(const IPASettings &settings, const InitParams ¶ms, InitResult *result) override;
> + int32_t configure(const IPACameraSensorInfo &sensorInfo, const ConfigParams ¶ms,
> + ConfigResult *result) override;
> +
> + void start(const ControlList &controls, StartResult *result) override;
> + void stop() override {}
> +
> + void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> + void unmapBuffers(const std::vector<unsigned int> &ids) override;
> +
> + void prepareIsp(const PrepareParams ¶ms) override;
> + void processStats(const ProcessParams ¶ms) override;
> +
> + void reportMetadata(unsigned int ipaContext, ControlList *controls) override;
> +
> +private:
> + /* Number of metadata objects available in the context list. */
> + static constexpr unsigned int numMetadataContexts = 16;
> +
> + virtual int32_t platformInit(const InitParams ¶ms, InitResult *result) = 0;
This enforces a particular sequence of calls between the base class and
the derived class, dictated by the base class design. It's best to
handle this in the derived class instead. You can override the init()
function in the derived class, call the init() function of the base
class, and then perform all the work you're currently doing in
IpaVc4::platformInit().
The same comment applies to the other platform*() functions (some may
require a little bit of refactoring). The handleControls() function can
also be dropped by making applyControls() a protected virtual function
that derived classes may override.
> + virtual int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) = 0;
> +
> + virtual void platformPrepareIsp(const PrepareParams ¶ms,
> + RPiController::Metadata &rpiMetadata) = 0;
> + virtual RPiController::StatisticsPtr platformProcessStats(Span<uint8_t> mem) = 0;
> +
> + void setMode(const IPACameraSensorInfo &sensorInfo);
> + void setCameraTimeoutValue();
> + bool validateSensorControls();
> + bool validateLensControls();
> + void applyControls(const ControlList &controls);
> + virtual void handleControls(const ControlList &controls) = 0;
> + void fillDeviceStatus(const ControlList &sensorControls, unsigned int ipaContext);
> + void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration);
> + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> +
> + std::map<unsigned int, MappedFrameBuffer> buffers_;
> +
> + bool lensPresent_;
> + ControlList libcameraMetadata_;
> +
> + std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;
> +
> + /*
> + * We count frames to decide if the frame must be hidden (e.g. from
> + * display) or mistrusted (i.e. not given to the control algos).
> + */
> + uint64_t frameCount_;
> +
> + /* How many frames we should avoid running control algos on. */
> + unsigned int mistrustCount_;
> +
> + /* Number of frames that need to be dropped on startup. */
> + unsigned int dropFrameCount_;
> +
> + /* Frame timestamp for the last run of the controller. */
> + uint64_t lastRunTimestamp_;
> +
> + /* Do we run a Controller::process() for this frame? */
> + bool processPending_;
> +
> + /* Distinguish the first camera start from others. */
> + bool firstStart_;
> +
> + /* Frame duration (1/fps) limits. */
> + utils::Duration minFrameDuration_;
> + utils::Duration maxFrameDuration_;
> +
> +protected:
We usually put protected members before private members.
> + /* Raspberry Pi controller specific defines. */
> + std::unique_ptr<RPiController::CamHelper> helper_;
> + RPiController::Controller controller_;
> +
> + ControlInfoMap sensorCtrls_;
> + ControlInfoMap lensCtrls_;
> +
> + /* Camera sensor params. */
> + CameraMode mode_;
> +
> + /* Track the frame length times over FrameLengthsQueueSize frames. */
> + std::deque<utils::Duration> frameLengths_;
> + utils::Duration lastTimeout_;
> +};
> +
> +} /* namespace ipa::RPi */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/rpi/common/meson.build b/src/ipa/rpi/common/meson.build
> new file mode 100644
> index 000000000000..a7189f8389af
> --- /dev/null
> +++ b/src/ipa/rpi/common/meson.build
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: CC0-1.0
> +
> +# SPDX-License-Identifier: CC0-1.0
One SPDX header is enough :-)
> +
> +rpi_ipa_common_sources = files([
> + 'ipa_base.cpp',
> +])
> diff --git a/src/ipa/rpi/vc4/meson.build b/src/ipa/rpi/vc4/meson.build
> index 992d0f1ab5a7..b581391586b3 100644
> --- a/src/ipa/rpi/vc4/meson.build
> +++ b/src/ipa/rpi/vc4/meson.build
> @@ -13,11 +13,15 @@ vc4_ipa_includes = [
> ]
>
> vc4_ipa_sources = files([
> - 'raspberrypi.cpp',
> + 'vc4.cpp',
> ])
>
> vc4_ipa_includes += include_directories('..')
> -vc4_ipa_sources += [rpi_ipa_cam_helper_sources, rpi_ipa_controller_sources]
> +vc4_ipa_sources += [
> + rpi_ipa_cam_helper_sources,
> + rpi_ipa_common_sources,
> + rpi_ipa_controller_sources,
Could we avoid compiling all these source files multiple times, once for
the IpaVc4 IPA module, and once for the other IPA modules that may be
implemented in the future ? This can be done with a static library, see
src/apps/common/meson.build and src/apps/cam/meson.build for instance.
> +]
>
> mod = shared_module(ipa_name,
> [vc4_ipa_sources, libcamera_generated_ipa_headers],
[snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list