[libcamera-devel] [PATCH 08/13] ipa: raspberrypi: Introduce IpaBase class
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Fri Apr 28 11:33:14 CEST 2023
Hi Naush
On Fri, Apr 28, 2023 at 09:35:51AM +0100, Naushir Patuck via libcamera-devel wrote:
> Hi Laurent,
>
> Thank you for your feedback.
>
> On Thu, 27 Apr 2023 at 16:54, Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
> >
> > 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/ ?
>
> 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. I admit I've researched
(briefly) what was the suggested way to handle methods composition in
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 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