[libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce IpaBase class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 2 17:35:11 CEST 2023
Hello,
On Fri, Apr 28, 2023 at 11:33:14AM +0200, Jacopo Mondi wrote:
> On Fri, Apr 28, 2023 at 09:35:51AM +0100, Naushir Patuck via libcamera-devel wrote:
> > On Thu, 27 Apr 2023 at 16:54, Laurent Pinchart wrote:
> > > 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/ ?
> >
> > I actually meant "Code for the implementation...", but I'll replace it
> > with your wording.
> >
> > > > 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 ?
> >
> > This was a mistake, I'll remove it.
> >
> > > > +
> > > > + 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().
> >
> > One of the earlier iterations had exactly this implementing. After
> > internal discussions, we decided to do it the other way round (i.e. the base
> > class implements the init() override). Out of curiosity, what advantages would
> > one way have over the other?
>
> I admit that it was my suggestion to pick this style as originally
> there was a mix of the two. Seeing an override calling into the base
> class' overridden method felt a bit weird.
That's a normal pattern in C++, base classes shouldn't, when possible,
imply a particular implementation of derived classes. Calling the base
class implementation at the beginning or end of a derived class'
function is perfectly fine and normal.
The other pattern, calling into a protected virtual function implemented
by the derived class, is an acceptable use case too.
> I admit I've researched
> (briefly) what was the suggested way to handle methods composition in
Note that composition and inheritance are two different things.
> C++ and found nothing relevant, so my thought was that it was better
> to clearly define what was the interface that derived classes need to
> implement, as the PipelineHandler/IPAInterface is implemented in the
> base classes.
>
> When it comes to the call order, it's the base class driving the
> procesing flow, while dispatching calls to the derived classes at the
> right time. Moving to your suggestion would allow derived classes to
> call the base class method, which means most of the control is moved
> to the derived classes with possible code duplication.
I think having the derived class in control is desirable, as only the
derived class knows about its specific needs. Otherwise you could have
to over-complicate the design of the base class to cover the needs of
different derived classes.
> I think to actually decide where to go, it is useful to analyze what
> class implements the operations flow control
>
> In one case
>
> Base::method() Derived::platformMethod()
>
> doSomeWork();
> ....
> platformMethod() --> doPlaformWork();
> ...
> finalizeWork();
>
> While in the other
>
> Base::method() Derived::method()
>
> doSomeWork();
> ....
> method() <-- Base::method()
> ...
> finalizeWork();
>
> Is this an over-simplification ?
>
> Naush, do you expect the derived classes to control the operation
> flow, or will this be common to both and could be implemented in the
> base classes ?
>
> > > 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],
> > >
> >
> > Sure, I'll fix this up.
> >
> > Naush
> >
> > > [snip]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list