[libcamera-devel] [PATCH v3 03/13] ipa: rkisp1: Support raw capture in IPA operations
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Oct 30 18:14:33 CET 2022
Hi Jacopo,
On Wed, Oct 26, 2022 at 04:18:01PM +0200, Jacopo Mondi wrote:
> On Mon, Oct 24, 2022 at 03:03:46AM +0300, Laurent Pinchart wrote:
> > The RkISP1 can capture raw frames by bypassing the ISP. In that mode,
> > the ISP will not produce statistics nor consume parameters. Most
> > algorithms will thus be unable to run, with one exception: the AGC will
> > still be able to configure the sensor exposure time and analog gain in
> > manual mode.
> >
> > To prepare for this, add the ability to disable algorithms for the
> > duration of the capture session based on the camera configuration.
> > Individual algorithms report whether they support raw formats at
> > construction time, and the IPA module disables algorithms in configure()
> > based on the stream configurations.
> >
> > Disabled algorithms are skipped during the capture session in the
> > processStatsBuffer() operation. As the ISP doesn't produce statistics,
> > don't try to access the stats buffer. There is no need for similar logic
> > in fillParamsBuffer() as that operation won't be called for raw capture.
> >
> > All algorithms report not supporting raw capture by default. Raw support
> > in AGC will be added separately.
> >
> > The feature is implemented in the RkISP1 module without any support from
> > libipa at this point to avoid designing a generic API based on a single
> > user. This may be changed in the future.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> While reading the patch I had the same thought I later found out Paul
> has already expressed.
>
> On making the algorithms self-disabling, I guess it would actually be
> nicer if they would handle it in configure() by returning immediately,
> just after having set their 'disabled' flag. However I understand it's
> a more invasive change, and all that encapsulation is not really
> necessary. Alternatively, the base class could do the check, but again
> it's not worth the complication most probably..
I've considered multiple options, and given that we have, for the
rkisp1, a single algorithm that can work in raw mode, I picked a
solution that didn't require all algorithms to be modified with raw
checks in configure(). I've also considered moving the infrastructure to
the base Algorithm class, but decided I didn't have enough IPA modules
supporting raw capture do design this properly. We may move it later,
when we'll have a second data point.
> A few minor nits below
>
> > ---
> > src/ipa/rkisp1/algorithms/algorithm.h | 12 +++++++-
> > src/ipa/rkisp1/ipa_context.cpp | 5 ++++
> > src/ipa/rkisp1/ipa_context.h | 2 ++
> > src/ipa/rkisp1/rkisp1.cpp | 42 +++++++++++++++++++++++----
> > 4 files changed, 54 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> > index c3212cff76fe..9454c9a1fc06 100644
> > --- a/src/ipa/rkisp1/algorithms/algorithm.h
> > +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> > @@ -15,7 +15,17 @@ namespace libcamera {
> >
> > namespace ipa::rkisp1 {
> >
> > -using Algorithm = libcamera::ipa::Algorithm<Module>;
> > +class Algorithm : public libcamera::ipa::Algorithm<Module>
> > +{
> > +public:
> > + Algorithm()
> > + : disabled_(false), supportsRaw_(false)
> > + {
> > + }
> > +
> > + bool disabled_;
> > + bool supportsRaw_;
> > +};
> >
> > } /* namespace ipa::rkisp1 */
> >
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 7a987497bd03..9bbf368432fa 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -89,6 +89,11 @@ namespace libcamera::ipa::rkisp1 {
> > * \brief Sensor output resolution
> > */
> >
> > +/**
> > + * \var IPASessionConfiguration::raw
> > + * \brief Indicates if the camera is configured to capture raw frames
> > + */
> > +
> > /**
> > * \struct IPAActiveState
> > * \brief Active state for algorithms
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index bb60ab9eab72..3e47ac663c58 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -48,6 +48,8 @@ struct IPASessionConfiguration {
> > struct {
> > rkisp1_cif_isp_version revision;
> > } hw;
> > +
> > + bool raw;
> > };
> >
> > struct IPAActiveState {
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index fcb9dacccc3c..538e42cb6f7f 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -24,6 +24,7 @@
> > #include <libcamera/ipa/rkisp1_ipa_interface.h>
> > #include <libcamera/request.h>
> >
> > +#include "libcamera/internal/formats.h"
> > #include "libcamera/internal/mapped_framebuffer.h"
> > #include "libcamera/internal/yaml_parser.h"
> >
> > @@ -207,7 +208,7 @@ void IPARkISP1::stop()
> > * before accessing them.
> > */
> > int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > - [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig,
> > + const std::map<uint32_t, IPAStream> &streamConfig,
> > const std::map<uint32_t, ControlInfoMap> &entityControls)
> > {
> > if (entityControls.empty())
> > @@ -264,7 +265,21 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > context_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain);
> > context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain);
> >
> > - for (auto const &algo : algorithms()) {
> > + context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(),
> > + [](auto &cfg) -> bool {
> > + PixelFormat pixelFormat{ cfg.second.pixelFormat };
>
> I guess there are no differences between using value contruction or
> aggregate construction when the argument is an int, I found () more
> natural..
Direct list initialization (T object { arg1, arg2, ... }) and direct
initialization (T object ( arg1, arg2, ... )) differ mostly in two ways
(for class types). First of all, list initialization will never perform
narrowing:
- An integer cannot be converted to another integer that cannot hold its
value. For example, char to int is allowed, but not int to char.
- A floating-point value cannot be converted to another floating-point
type that cannot hold its value. For example, float to double is
allowed, but not double to float.
- A floating-point value cannot be converted to an integer type.
- An integer value cannot be converted to a floating-point type.
Then, list initialization will first try constructors that take a
std::initializer_list as a single argument, so
std::vector<int> v1(10, 20);
and
std::vector<int> v1{10, 20};
lead to different results. The former creates a vector of 10 elements
initialized to value 20, while the latter creates a vector of 2
elements.
This often lead to recommending list initialization by default unless
there is a reason to do otherwise. We don't have a clear default rule in
libcamera, maybe we should ?
> > + const PixelFormatInfo &format = PixelFormatInfo::info(pixelFormat);
> > + return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> > + });
> > +
> > + for (auto const &a : algorithms()) {
> > + Algorithm *algo = static_cast<Algorithm *>(a.get());
> > +
> > + /* Disable algorithms that don't support raw formats. */
> > + algo->disabled_ = context_.configuration.raw && !algo->supportsRaw_;
> > + if (algo->disabled_)
> > + continue;
> > +
> > int ret = algo->configure(context_, info);
> > if (ret)
> > return ret;
> > @@ -307,8 +322,12 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)
> > {
> > IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
> >
> > - for (auto const &algo : algorithms())
> > + for (auto const &a : algorithms()) {
> > + Algorithm *algo = static_cast<Algorithm *>(a.get());
> > + if (algo->disabled_)
> > + continue;
> > algo->queueRequest(context_, frame, frameContext, controls);
> > + }
> > }
> >
> > void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
> > @@ -333,9 +352,16 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> > {
> > IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> >
> > - const rkisp1_stat_buffer *stats =
> > - reinterpret_cast<rkisp1_stat_buffer *>(
> > + /*
> > + * In raw capture mode, the ISP is bypassed and no statistics buffer is
> > + * provided.
> > + */
> > + const rkisp1_stat_buffer *stats;
> > + if (!context_.configuration.raw)
> > + stats = reinterpret_cast<rkisp1_stat_buffer *>(
> > mappedBuffers_.at(bufferId).planes()[0].data());
> > + else
> > + stats = nullptr;
> >
>
> Or
> const rkisp1_stat_buffer *stats = nullptr;
> if (!context_.configuration.raw)
> stats = reinterpret_cast<rkisp1_stat_buffer *>(
> mappedBuffers_.at(bufferId).planes()[0].data());
I don't mind much either way.
> All minors and mostly personal preferences
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > frameContext.sensor.exposure =
> > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> > @@ -344,8 +370,12 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> >
> > ControlList metadata(controls::controls);
> >
> > - for (auto const &algo : algorithms())
> > + for (auto const &a : algorithms()) {
> > + Algorithm *algo = static_cast<Algorithm *>(a.get());
> > + if (algo->disabled_)
> > + continue;
> > algo->process(context_, frame, frameContext, stats, metadata);
> > + }
> >
> > setControls(frame);
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list