From cbb5740441f2ab8509b4e3c0182fd31c73955761 Mon Sep 17 00:00:00 2001 From: Paul Elder Date: Thu, 21 Jul 2022 13:12:59 +0100 Subject: [PATCH 01/15] libcamera: request: Add support for error flags Add error flags to the Request to indicate non-fatal errors. Applications should check the error() state of the Request to determine if any fault occured while processing the request. Initially, a single error flag ControlError is added to allow a pipeline handler to report a failure to set controls on a hardware device while processing the request. ControlErrors occur when a Request was asked to set a control and the pipeline handler attempted to do so, but it was rejected by the kernel. Signed-off-by: Paul Elder Signed-off-by: Kieran Bingham --- include/libcamera/internal/request.h | 3 ++ include/libcamera/request.h | 9 ++++++ src/libcamera/request.cpp | 48 ++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+) diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h index 9dadd6c6..8e592272 100644 --- a/include/libcamera/internal/request.h +++ b/include/libcamera/internal/request.h @@ -38,6 +38,7 @@ class Request::Private : public Extensible::Private void complete(); void cancel(); void reuse(); + void setErrorFlags(ErrorFlags flags); void prepare(std::chrono::milliseconds timeout = 0ms); Signal<> prepared; @@ -59,6 +60,8 @@ class Request::Private : public Extensible::Private std::unordered_set pending_; std::map> notifiers_; std::unique_ptr timer_; + + ErrorFlags error_; }; } /* namespace libcamera */ diff --git a/include/libcamera/request.h b/include/libcamera/request.h index dffde153..992629e1 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -43,6 +44,13 @@ class Request : public Extensible ReuseBuffers = (1 << 0), }; + enum ErrorFlag { + NoError = 0, + ControlError = (1 << 0), + }; + + using ErrorFlags = Flags; + using BufferMap = std::map; Request(Camera *camera, uint64_t cookie = 0); @@ -60,6 +68,7 @@ class Request : public Extensible uint32_t sequence() const; uint64_t cookie() const { return cookie_; } Status status() const { return status_; } + ErrorFlags error() const; bool hasPendingBuffers() const; diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index d2af1d22..a24ee397 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -162,6 +162,7 @@ void Request::Private::cancel() */ void Request::Private::reuse() { + error_ = Request::NoError; sequence_ = 0; cancelled_ = false; prepared_ = false; @@ -284,6 +285,21 @@ void Request::Private::notifierActivated(FrameBuffer *buffer) emitPrepareCompleted(); } +/** + * \brief Update the error flags of the Request + * \param[in] flags Flags to apply on the Request + * + * Apply \a flags to the Request to report to the application when the Request + * completes. + * + * Setting an Error flag does not cause a Request to fail, and once set it can + * only be cleared by the application destroying the Request or calling reuse(). + */ +void Request::Private::setErrorFlags(ErrorFlags flags) +{ + error_ |= flags; +} + void Request::Private::timeout() { /* A timeout can only happen if there are fences not yet signalled. */ @@ -318,6 +334,24 @@ void Request::Private::timeout() * Reuse the buffers that were previously added by addBuffer() */ +/** + * \enum Request::ErrorFlag + * Flags to report non-fatal errors + * + * \var Request::NoError + * No error + * + * \var Request::ControlError + * Control Error. At least on control was not able to be applied to the device. + * The application should compare the metadata to the requested control values + * to check which controls weren't applied. + */ + +/** + * \typedef Request::ErrorFlags + * The error state of the request defined by \a Request::ErrorFlag + */ + /** * \typedef Request::BufferMap * \brief A map of Stream to FrameBuffer pointers @@ -560,6 +594,20 @@ uint32_t Request::sequence() const * \return The request completion status */ +/** + * \brief Retrieve the error flags + * + * The request could complete with non-fatal error. The completion status will + * indicate success. This function returns the non-fatal errors that the + * request completed with + * + * \return Flags of non-fatal errors that the request completed with + */ +Request::ErrorFlags Request::error() const +{ + return _d()->error_; +} + /** * \brief Check if a request has buffers yet to be completed * From e56fb9944a10b702c2145ae0847641846797ba6c Mon Sep 17 00:00:00 2001 From: Jacopo Mondi Date: Thu, 4 Aug 2022 14:30:57 +0200 Subject: [PATCH 02/15] fixup: libcamera: request: Add support for error flags Address review comments from Umang and - Use Request::ErrorId and Request::Errors - Reword documentation of enum members as suggested - Rework documentation of Request::errors() - Update documentation of Request::status() to remove the term 'errors' from there Signed-off-by: Jacopo Mondi --- include/libcamera/internal/request.h | 5 ++- include/libcamera/request.h | 6 +-- src/libcamera/request.cpp | 60 ++++++++++++++++++---------- 3 files changed, 44 insertions(+), 27 deletions(-) diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h index 8e592272..e51aee8c 100644 --- a/include/libcamera/internal/request.h +++ b/include/libcamera/internal/request.h @@ -38,11 +38,12 @@ class Request::Private : public Extensible::Private void complete(); void cancel(); void reuse(); - void setErrorFlags(ErrorFlags flags); void prepare(std::chrono::milliseconds timeout = 0ms); Signal<> prepared; + void setError(Errors error); + private: friend class PipelineHandler; friend std::ostream &operator<<(std::ostream &out, const Request &r); @@ -61,7 +62,7 @@ class Request::Private : public Extensible::Private std::map> notifiers_; std::unique_ptr timer_; - ErrorFlags error_; + Errors error_; }; } /* namespace libcamera */ diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 992629e1..dde93322 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -44,12 +44,12 @@ class Request : public Extensible ReuseBuffers = (1 << 0), }; - enum ErrorFlag { + enum ErrorId { NoError = 0, ControlError = (1 << 0), }; - using ErrorFlags = Flags; + using Errors = Flags; using BufferMap = std::map; @@ -68,7 +68,7 @@ class Request : public Extensible uint32_t sequence() const; uint64_t cookie() const { return cookie_; } Status status() const { return status_; } - ErrorFlags error() const; + Errors error() const; bool hasPendingBuffers() const; diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index a24ee397..c7fcc6ad 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -287,17 +287,17 @@ void Request::Private::notifierActivated(FrameBuffer *buffer) /** * \brief Update the error flags of the Request - * \param[in] flags Flags to apply on the Request + * \param[in] error Error flags to apply on the Request * - * Apply \a flags to the Request to report to the application when the Request - * completes. + * Flag \a error in the Request which will get reported to the application when + * the Request completes. * - * Setting an Error flag does not cause a Request to fail, and once set it can + * Setting an error flag does not cause a Request to fail, and once set it can * only be cleared by the application destroying the Request or calling reuse(). */ -void Request::Private::setErrorFlags(ErrorFlags flags) +void Request::Private::setError(Errors error) { - error_ |= flags; + error_ |= error; } void Request::Private::timeout() @@ -335,21 +335,22 @@ void Request::Private::timeout() */ /** - * \enum Request::ErrorFlag - * Flags to report non-fatal errors + * \enum Request::ErrorId + * Flags to report errors on a completed request * * \var Request::NoError - * No error + * No error. The Request completed succesfully and all its buffer contain + * valid data * * \var Request::ControlError - * Control Error. At least on control was not able to be applied to the device. + * Control Error. At least one control was not applied correctly to the camera. * The application should compare the metadata to the requested control values * to check which controls weren't applied. */ /** - * \typedef Request::ErrorFlags - * The error state of the request defined by \a Request::ErrorFlag + * \typedef Request::Errors + * The error state of the request defined by \a Request::ErrorId */ /** @@ -586,24 +587,39 @@ uint32_t Request::sequence() const * \brief Retrieve the request completion status * * The request status indicates whether the request has completed successfully - * or with an error. When requests are created and before they complete the - * request status is set to RequestPending, and is updated at completion time - * to RequestComplete. If a request is cancelled at capture stop before it has - * completed, its status is set to RequestCancelled. + * or has been cancelled before being processed. + * + * Requests are created with their status set to RequestPending. When + * a Request is successfully processed and completed by the Camera its + * status is set to RequestComplete. If a Request is cancelled before + * being processed, for example because the Camera has been stopped + * before the request is completed, its status is set to RequestCancelled. + * + * Successfully completed requests can complete with errors. Applications shall + * inspect the error mask returned by Request::error() before accessing buffers + * and data associated with a completed request. * * \return The request completion status */ /** - * \brief Retrieve the error flags + * \brief Retrieve the mask of error flags associated with a completed request + * + * The request could complete with errors, which indicate failures in + * completing correctly parts of the request submitted by the application. + * + * The possible failure reasons are defined by the error flags defined + * by Request::ErrorId and application are expected to retrieve the + * mask of error flags by using this function before accessing the + * buffers and data associated with a completed request. * - * The request could complete with non-fatal error. The completion status will - * indicate success. This function returns the non-fatal errors that the - * request completed with + * Error conditions reported through this function do not change the + * request completion status retrieved through Request::status() which + * indicates if the Request has been processed or not. * - * \return Flags of non-fatal errors that the request completed with + * \return A mask of error identifier with which the request was completed */ -Request::ErrorFlags Request::error() const +Request::Errors Request::error() const { return _d()->error_; } From 2b4531f608aa302b5f1691354f23e0fd04dd42f0 Mon Sep 17 00:00:00 2001 From: Kieran Bingham via libcamera-devel Date: Thu, 21 Jul 2022 13:13:00 +0100 Subject: [PATCH 03/15] libcamera: pipeline: uvcvideo: Report control errors Report an error when failing to process controls, but still allow the request to process and complete where possible. The Request ControlError flag is raised on the request. Bug: https://bugs.libcamera.org/show_bug.cgi?id=135 Signed-off-by: Kieran Bingham Reviewed-by: Umang Jain Reviewed-by: Jacopo Mondi Signed-off-by: Jacopo Mondi --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index fbe02cdc..8ffa27b1 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -26,6 +26,7 @@ #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/pipeline_handler.h" +#include "libcamera/internal/request.h" #include "libcamera/internal/sysfs.h" #include "libcamera/internal/v4l2_videodevice.h" @@ -373,8 +374,10 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) } int ret = processControls(data, request); - if (ret < 0) - return ret; + if (ret < 0) { + LOG(UVC, Error) << "Failed to process controls"; + request->_d()->setError(Request::ControlError); + } ret = data->video_->queueBuffer(buffer); if (ret < 0) From ed21439231f1d3b91d3be26dee1181c786d034c1 Mon Sep 17 00:00:00 2001 From: Kieran Bingham via libcamera-devel Date: Thu, 21 Jul 2022 13:13:01 +0100 Subject: [PATCH 04/15] libcamera: request: Add PFCError flag Provide a new flag for the Request error state to indicate that a per-frame control error has occurred. One or more controls set for this request could not be guaranteed on this frame. Signed-off-by: Kieran Bingham Reviewed-by: Jacopo Mondi Signed-off-by: Jacopo Mondi --- include/libcamera/request.h | 1 + src/libcamera/request.cpp | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/include/libcamera/request.h b/include/libcamera/request.h index dde93322..2f716123 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -47,6 +47,7 @@ class Request : public Extensible enum ErrorId { NoError = 0, ControlError = (1 << 0), + PFCError = (1 << 1), }; using Errors = Flags; diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index c7fcc6ad..18dfdcc1 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -346,6 +346,10 @@ void Request::Private::timeout() * Control Error. At least one control was not applied correctly to the camera. * The application should compare the metadata to the requested control values * to check which controls weren't applied. + * + * \var Request::PFCError + * A per-frame-control error has occured. Controls that were expected to be set + * during the processing of this request were not processed in time. */ /** From 98bde7ddc065f274dcc3a97d11ffbf86eb28f258 Mon Sep 17 00:00:00 2001 From: Umang Jain Date: Thu, 21 Jul 2022 13:13:02 +0100 Subject: [PATCH 05/15] ipa: ipu3: Move the Frame Context queue to libipa Provide a direct FCQueue abstraction to facilitate creating a queue of Frame Context structures. The IPAFrameContext is moved back to the ipu3/ipa_context.h and a queue added to the IPU3, but the Algorithms are not yet moved from the single FrameContext exposed by the IPA context to use the correct one from the queue until the Algorithm interfaces are fully updated. Signed-off-by: Umang Jain Signed-off-by: Kieran Bingham Signed-off-by: Jacopo Mondi --- src/ipa/ipu3/ipa_context.cpp | 20 ++------ src/ipa/ipu3/ipa_context.h | 16 +++--- src/ipa/ipu3/ipu3.cpp | 13 +++-- src/ipa/libipa/fc_queue.cpp | 96 ++++++++++++++++++++++++++++++++++++ src/ipa/libipa/fc_queue.h | 94 +++++++++++++++++++++++++++++++++++ src/ipa/libipa/meson.build | 2 + 6 files changed, 210 insertions(+), 31 deletions(-) create mode 100644 src/ipa/libipa/fc_queue.cpp create mode 100644 src/ipa/libipa/fc_queue.h diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp index 13cdb835..9605c8a1 100644 --- a/src/ipa/ipu3/ipa_context.cpp +++ b/src/ipa/ipu3/ipa_context.cpp @@ -180,27 +180,10 @@ namespace libcamera::ipa::ipu3 { * struct ipu3_uapi_gamma_corr_lut for further details. */ -/** - * \brief Default constructor for IPAFrameContext - */ -IPAFrameContext::IPAFrameContext() = default; - -/** - * \brief Construct a IPAFrameContext instance - */ -IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) - : frame(id), frameControls(reqControls) -{ - sensor = {}; -} - /** * \var IPAFrameContext::frame * \brief The frame number * - * \var IPAFrameContext::frameControls - * \brief Controls sent in by the application while queuing the request - * * \var IPAFrameContext::sensor * \brief Effective sensor values that were applied for the frame * @@ -209,6 +192,9 @@ IPAFrameContext::IPAFrameContext(uint32_t id, const ControlList &reqControls) * * \var IPAFrameContext::sensor.gain * \brief Analogue gain multiplier + * + * \var IPAFrameContext::error + * \brief The error flags for this frame context */ } /* namespace libcamera::ipa::ipu3 */ diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h index 42e11141..dc5b7c30 100644 --- a/src/ipa/ipu3/ipa_context.h +++ b/src/ipa/ipu3/ipa_context.h @@ -8,22 +8,20 @@ #pragma once -#include - #include #include #include #include +#include + +#include namespace libcamera { namespace ipa::ipu3 { -/* Maximum number of frame contexts to be held */ -static constexpr uint32_t kMaxFrameContexts = 16; - struct IPASessionConfiguration { struct { ipu3_uapi_grid_config bdsGrid; @@ -77,23 +75,21 @@ struct IPAActiveState { }; struct IPAFrameContext { - IPAFrameContext(); - IPAFrameContext(uint32_t id, const ControlList &reqControls); + uint32_t frame; struct { uint32_t exposure; double gain; } sensor; - uint32_t frame; - ControlList frameControls; + Request::Errors error; }; struct IPAContext { IPASessionConfiguration configuration; IPAActiveState activeState; - std::array frameContexts; + FCQueue frameContexts; }; } /* namespace ipa::ipu3 */ diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 2f6bb672..55e82cd4 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -38,6 +38,8 @@ #include "algorithms/tone_mapping.h" #include "libipa/camera_sensor_helper.h" +#include "ipa_context.h" + /* Minimum grid width, expressed as a number of cells */ static constexpr uint32_t kMinGridWidth = 16; /* Maximum grid width, expressed as a number of cells */ @@ -456,8 +458,7 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, /* Clean IPAActiveState at each reconfiguration. */ context_.activeState = {}; - IPAFrameContext initFrameContext; - context_.frameContexts.fill(initFrameContext); + context_.frameContexts.clear(); if (!validateSensorControls()) { LOG(IPAIPU3, Error) << "Sensor control validation failed."; @@ -569,7 +570,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, const ipu3_uapi_stats_3a *stats = reinterpret_cast(mem.data()); - IPAFrameContext &frameContext = context_.frameContexts[frame % kMaxFrameContexts]; + IPAFrameContext &frameContext = context_.frameContexts.get(frame); if (frameContext.frame != frame) LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context"; @@ -618,7 +619,11 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) { /* \todo Start processing for 'frame' based on 'controls'. */ - context_.frameContexts[frame % kMaxFrameContexts] = { frame, controls }; + IPAFrameContext &frameContext = context_.frameContexts.initialise(frame); + + /* \todo Implement queueRequest to each algorithm. */ + (void)frameContext; + (void)controls; } /** diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp new file mode 100644 index 00000000..ecb47f28 --- /dev/null +++ b/src/ipa/libipa/fc_queue.cpp @@ -0,0 +1,96 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2022, Google Inc. + * + * fc_queue.cpp - IPA Frame context queue + */ + +#include "fc_queue.h" + +#include + +namespace libcamera { + +LOG_DEFINE_CATEGORY(FCQueue) + +namespace ipa { + +/** + * \file fc_queue.h + * \brief Queue to access per-frame Context + */ + +/** + * \class FCQueue + * \brief A support class for queueing Frame Context instances through the IPA + * \tparam FrameContext The IPA specific Frame Context type for this queue + * + * The Frame Context Queue provides a simple circular buffer implementation to + * store IPA specific context for each frame through its lifetime within the + * IPA. + * + * FrameContexts are expected to be referenced by a monotonically increasing + * sequence count referring to a Frame sequence. + * + * A FrameContext is initialised for a given frame when the corresponding + * Request is first queued into the IPA. From that point on the FrameContext can + * be obtained by the IPA and its algorithms by referencing it from the frame + * sequence number. + * + * A frame sequence number from the image source must correspond to the request + * sequence number for this implementation to be supported in an IPA. It is + * expected that the same sequence number will be used to reference the context + * of the Frame from the point of queueing the request, specifying controls for + * a given frame, and processing of any ISP related controls and statistics for + * the same corresponding image. + * + * IPA specific FrameContexts should inherit from the IPAFrameContext to support + * the minimum required features for a FrameContext, including the frame number + * and error flags that relate to the frame. + * + * FrameContexts are overwritten when they are recycled and re-initialised by + * the first access made on them by either initialise(frame) or get(frame). If a + * FrameContext is first accessed through get(frame) before calling initialise() + * a PFCError is automatically raised on the FrameContext to be transferred to + * the Request when it completes. + */ + +/** + * \fn FCQueue::clear() + * \brief Reinitialise all data on the queue + * + * Reset the queue and ensure all FrameContext slots are re-initialised. + */ + +/** + * \fn FCQueue::initialise(uint32_t frame) + * \brief Initialize and return the Frame Context at slot specified by \a frame + * \param[in] frame The frame context sequence number + * + * The first call to obtain a FrameContext from the FCQueue should be handled + * through this call. The FrameContext will be initialised for the frame and + * returned to the caller if it was not already initialised. + * + * If the FrameContext was already initialized for this sequence, a warning will + * be reported and the previously initialized FrameContext is returned. + * + * \return A reference to the FrameContext for sequence \a frame + */ + +/** + * \fn FCQueue::get() + * \brief Obtain the Frame Context at slot specified by \a frame + * \param[in] frame The frame context sequence number + * + * Obtains an existing FrameContext from the queue and returns it to the caller. + * + * If the FrameContext is not correctly intiialised for the \a frame, it will be + * initialised and a PFCError will be flagged on the context to be transferred + * to the Request when it completes. + * + * \return A reference to the FrameContext for sequence \a frame + */ + +} /* namespace ipa */ + +} /* namespace libcamera */ diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h new file mode 100644 index 00000000..76868e28 --- /dev/null +++ b/src/ipa/libipa/fc_queue.h @@ -0,0 +1,94 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2022, Google Inc. + * + * fc_queue.h - IPA Frame context queue + * + */ + +#pragma once + +#include + +#include + +#include +#include + +namespace libcamera { + +LOG_DECLARE_CATEGORY(FCQueue) + +namespace ipa { + +/* + * Maximum number of frame contexts to be held onto + * + * \todo Should be larger than ISP delay + sensor delay + */ +static constexpr uint32_t kMaxFrameContexts = 16; + +template +class FCQueue : public std::array +{ +private: + void initialise(FrameContext &frameContext, const uint32_t frame) + { + frameContext = {}; + frameContext.frame = frame; + } + +public: + void clear() + { + this->fill({}); + } + + FrameContext &initialise(const uint32_t frame) + { + FrameContext &frameContext = this->at(frame & kMaxFrameContexts); + + /* + * Do not re-initialise if a get() call has already fetched this + * frame context to preseve the error flags already raised. + */ + if (frame == frameContext.frame && frame != 0) { + LOG(FCQueue, Warning) + << "Frame " << frame << " already initialised"; + } else { + initialise(frameContext, frame); + } + + return frameContext; + } + + FrameContext &get(uint32_t frame) + { + FrameContext &frameContext = this->at(frame & kMaxFrameContexts); + + if (frame != frameContext.frame) { + LOG(FCQueue, Warning) + << "Obtained an uninitialised FrameContext for " + << frame; + + initialise(frameContext, frame); + + /* + * The frame context has been retrieved before it was + * initialised through the initialise() call. This + * indicates an algorithm attempted to access a Frame + * context before it was queued to the IPA. + * + * Controls applied for this request may be left + * unhandled. + */ + frameContext.error |= Request::PFCError; + } + + return frameContext; + } +}; + +} /* namespace ipa */ + +} /* namespace libcamera */ diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build index fb894bc6..016b8e0e 100644 --- a/src/ipa/libipa/meson.build +++ b/src/ipa/libipa/meson.build @@ -3,6 +3,7 @@ libipa_headers = files([ 'algorithm.h', 'camera_sensor_helper.h', + 'fc_queue.h', 'histogram.h', 'module.h', ]) @@ -10,6 +11,7 @@ libipa_headers = files([ libipa_sources = files([ 'algorithm.cpp', 'camera_sensor_helper.cpp', + 'fc_queue.cpp', 'histogram.cpp', 'module.cpp', ]) From 4d3364d21e46a5d41acc644ea556b22743b1aca9 Mon Sep 17 00:00:00 2001 From: Jacopo Mondi Date: Thu, 4 Aug 2022 15:01:33 +0200 Subject: [PATCH 06/15] fixup: Address Umang's review comments --- src/ipa/libipa/fc_queue.cpp | 2 +- src/ipa/libipa/fc_queue.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp index ecb47f28..24522eba 100644 --- a/src/ipa/libipa/fc_queue.cpp +++ b/src/ipa/libipa/fc_queue.cpp @@ -84,7 +84,7 @@ namespace ipa { * * Obtains an existing FrameContext from the queue and returns it to the caller. * - * If the FrameContext is not correctly intiialised for the \a frame, it will be + * If the FrameContext is not correctly initialised for the \a frame, it will be * initialised and a PFCError will be flagged on the context to be transferred * to the Request when it completes. * diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h index 76868e28..4a0b1496 100644 --- a/src/ipa/libipa/fc_queue.h +++ b/src/ipa/libipa/fc_queue.h @@ -29,7 +29,7 @@ namespace ipa { static constexpr uint32_t kMaxFrameContexts = 16; template -class FCQueue : public std::array +class FCQueue : private std::array { private: void initialise(FrameContext &frameContext, const uint32_t frame) From e9e354a8d91a4798dab4cff8411f6347e8bf0c8a Mon Sep 17 00:00:00 2001 From: Jacopo Mondi Date: Thu, 4 Aug 2022 15:03:55 +0200 Subject: [PATCH 07/15] fixup: Clean context at IPA::stop() Doing the same at IPA::start() could conflict with queueRequest(). --- src/ipa/ipu3/ipu3.cpp | 7 +++---- src/ipa/rkisp1/rkisp1.cpp | 14 ++++++++++---- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 55e82cd4..209a6b04 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -350,6 +350,9 @@ int IPAIPU3::start() */ void IPAIPU3::stop() { + /* Clean the IPA context at the end of the streaming session. */ + context_.frameContexts.clear(); + context_ = {}; } /** @@ -456,10 +459,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo, calculateBdsGrid(configInfo.bdsOutputSize); - /* Clean IPAActiveState at each reconfiguration. */ - context_.activeState = {}; - context_.frameContexts.clear(); - if (!validateSensorControls()) { LOG(IPAIPU3, Error) << "Sensor control validation failed."; return -EINVAL; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 6cf4d169..0873d7fe 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -49,7 +49,7 @@ class IPARkISP1 : public IPARkISP1Interface, public Module int init(const IPASettings &settings, unsigned int hwRevision, ControlInfoMap *ipaControls) override; int start() override; - void stop() override {} + void stop() override; int configure(const IPACameraSensorInfo &info, const std::map &streamConfig, @@ -184,11 +184,20 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, int IPARkISP1::start() { + /* Clean the IPA context before starting the streaming session. */ + context_ = {}; + setControls(0); return 0; } +void IPARkISP1::stop() +{ + /* Clean the IPA context at the end of the streaming session. */ + context_ = {}; +} + /** * \todo The RkISP1 pipeline currently provides an empty IPACameraSensorInfo * if the connected sensor does not provide enough information to properly @@ -228,9 +237,6 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, << "Exposure: " << minExposure << "-" << maxExposure << " Gain: " << minGain << "-" << maxGain; - /* Clean context at configuration */ - context_ = {}; - /* Set the hardware revision for the algorithms. */ context_.configuration.hw.revision = hwRevision_; From db69b09bfb9e016cf34a339536ce288840746b28 Mon Sep 17 00:00:00 2001 From: Jacopo Mondi Date: Thu, 4 Aug 2022 15:20:51 +0200 Subject: [PATCH 08/15] fixup: Apply minor review comments Apply review comments on documentation and FCQueue implementation. --- src/ipa/libipa/fc_queue.cpp | 42 +++++++++++++++++++++++++------------ src/ipa/libipa/fc_queue.h | 40 ++++++++++++++++------------------- 2 files changed, 47 insertions(+), 35 deletions(-) diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp index 24522eba..37ffca60 100644 --- a/src/ipa/libipa/fc_queue.cpp +++ b/src/ipa/libipa/fc_queue.cpp @@ -22,15 +22,15 @@ namespace ipa { /** * \class FCQueue - * \brief A support class for queueing Frame Context instances through the IPA - * \tparam FrameContext The IPA specific Frame Context type for this queue + * \brief A support class for queueing FrameContext instances through the IPA + * \tparam FrameContext The IPA specific FrameContext derived class type * - * The Frame Context Queue provides a simple circular buffer implementation to + * The frame context queue provides a simple circular buffer implementation to * store IPA specific context for each frame through its lifetime within the * IPA. * - * FrameContexts are expected to be referenced by a monotonically increasing - * sequence count referring to a Frame sequence. + * FrameContext instances are expected to be referenced by a monotonically + * increasing sequence count corresponding to a frame sequence number. * * A FrameContext is initialised for a given frame when the corresponding * Request is first queued into the IPA. From that point on the FrameContext can @@ -44,22 +44,35 @@ namespace ipa { * a given frame, and processing of any ISP related controls and statistics for * the same corresponding image. * - * IPA specific FrameContexts should inherit from the IPAFrameContext to support - * the minimum required features for a FrameContext, including the frame number - * and error flags that relate to the frame. + * IPA specific frame context implementations shall inherit from the + * IPAFrameContext base class to support the minimum required features for a + * FrameContext, including the frame number and the error flags that relate to + * the frame. * * FrameContexts are overwritten when they are recycled and re-initialised by - * the first access made on them by either initialise(frame) or get(frame). If a - * FrameContext is first accessed through get(frame) before calling initialise() - * a PFCError is automatically raised on the FrameContext to be transferred to - * the Request when it completes. + * the first access made on them by either initialise(frame) or get(frame). It + * is required that the number of available slots in the frame context queue is + * larger or equal to the maximum number of in-flight requests a pipeline can + * handle to avoid overwriting frame context instances that still have to be + * processed. + * + * In the case an application does not queue requests to the Camera fast + * enough, frames can be produced and processed by the IPA without a + * corresponding Request being queued. In this case the IPA algorithm + * will try to access the FrameContext with a call to get() before it + * had been initialized at queueRequest() time. In this case there + * is now way the controls associated with the late Request could be + * applied in time, as the frame as already been processed by the IPA, + * the PFCError flag is automatically raised on the FrameContext. */ /** * \fn FCQueue::clear() - * \brief Reinitialise all data on the queue + * \brief Clear the context queue * * Reset the queue and ensure all FrameContext slots are re-initialised. + * IPA modules are expected to clear the frame context queue at the beginning of + * a new streaming session, in IPAModule::start(). */ /** @@ -74,6 +87,9 @@ namespace ipa { * If the FrameContext was already initialized for this sequence, a warning will * be reported and the previously initialized FrameContext is returned. * + * Frame contexts are expected to be initialised when a Request is first passed + * to the IPA module in IPAModule::queueRequest(). + * * \return A reference to the FrameContext for sequence \a frame */ diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h index 4a0b1496..c78703b6 100644 --- a/src/ipa/libipa/fc_queue.h +++ b/src/ipa/libipa/fc_queue.h @@ -3,7 +3,6 @@ * Copyright (C) 2022, Google Inc. * * fc_queue.h - IPA Frame context queue - * */ #pragma once @@ -12,7 +11,6 @@ #include -#include #include namespace libcamera { @@ -24,7 +22,7 @@ namespace ipa { /* * Maximum number of frame contexts to be held onto * - * \todo Should be larger than ISP delay + sensor delay + * \todo Should be platform-specific and match the pipeline depth */ static constexpr uint32_t kMaxFrameContexts = 16; @@ -52,12 +50,11 @@ class FCQueue : private std::array * Do not re-initialise if a get() call has already fetched this * frame context to preseve the error flags already raised. */ - if (frame == frameContext.frame && frame != 0) { + if (frame != 0 && frame <= frameContext.frame) LOG(FCQueue, Warning) << "Frame " << frame << " already initialised"; - } else { + else initialise(frameContext, frame); - } return frameContext; } @@ -66,24 +63,23 @@ class FCQueue : private std::array { FrameContext &frameContext = this->at(frame & kMaxFrameContexts); - if (frame != frameContext.frame) { - LOG(FCQueue, Warning) - << "Obtained an uninitialised FrameContext for " - << frame; + if (frame == frameContext.frame) + return frameContext; - initialise(frameContext, frame); + LOG(FCQueue, Warning) + << "Obtained an uninitialised FrameContext for " << frame; - /* - * The frame context has been retrieved before it was - * initialised through the initialise() call. This - * indicates an algorithm attempted to access a Frame - * context before it was queued to the IPA. - * - * Controls applied for this request may be left - * unhandled. - */ - frameContext.error |= Request::PFCError; - } + initialise(frameContext, frame); + + /* + * The frame context has been retrieved before it was + * initialised through the initialise() call. This indicates an + * algorithm attempted to access a Frame context before it was + * queued to the IPA. + * + * Controls applied for this request may be left unhandled. + */ + frameContext.error |= Request::PFCError; return frameContext; } From d351a6a9ef89a760584beaf5122c73bd41123905 Mon Sep 17 00:00:00 2001 From: Jacopo Mondi Date: Thu, 4 Aug 2022 15:40:57 +0200 Subject: [PATCH 09/15] fixup: Harden frame context overwrite error condition Harden the error condition in the case the desired frame context has been forever lost as too many requests has been queued by the pipeline handler. As this is a programming error, it is fair to ASSERT() on it. While at it expand the comment on the opposite case, where a request underrun makes the queue overflow and record that we might possibily want to notify that to applications. Signed-off-by: Jacopo Mondi --- src/ipa/libipa/fc_queue.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h index c78703b6..c385d42e 100644 --- a/src/ipa/libipa/fc_queue.h +++ b/src/ipa/libipa/fc_queue.h @@ -49,6 +49,13 @@ class FCQueue : private std::array /* * Do not re-initialise if a get() call has already fetched this * frame context to preseve the error flags already raised. + * + * \todo If the the sequence number of the context to initialise + * is smaller than the sequence number of the queue slot to use, + * it means that we had a serious request underrun and more + * frames than the queue size has been produced since the last + * time the application has queued a request. Does this deserve + * an error condition ? */ if (frame != 0 && frame <= frameContext.frame) LOG(FCQueue, Warning) @@ -63,6 +70,18 @@ class FCQueue : private std::array { FrameContext &frameContext = this->at(frame & kMaxFrameContexts); + /* + * If the IPA algorithms try to access a frame context slot which + * has been already overwritten by a newer context, it means the + * frame context queue has overflowed and the desired context + * has been forever lost. The pipeline handler shall avoid + * queueing more requests to the IPA than the frame context + * queue size. + */ + if (frame < frameContext.frame) + LOG(FCQueue, Fatal) + << "Frame " << frame << " has been overwritten"; + if (frame == frameContext.frame) return frameContext; From d9de4f70044d1d00acc8a9f6f93ee6d50f5b3219 Mon Sep 17 00:00:00 2001 From: Kieran Bingham via libcamera-devel Date: Thu, 21 Jul 2022 13:13:03 +0100 Subject: [PATCH 10/15] ipa: rkisp1: Rename frameContext to activeState Move the existing frame context structure to the IPAActiveState. This structure should store the most up to date results for the IPA which may be shared to other algorithms that operate on the data. Signed-off-by: Kieran Bingham Reviewed-by: Umang Jain Reviewed-by: Jacopo Mondi --- src/ipa/rkisp1/algorithms/agc.cpp | 20 ++++++++-------- src/ipa/rkisp1/algorithms/awb.cpp | 34 ++++++++++++++-------------- src/ipa/rkisp1/algorithms/blc.cpp | 2 +- src/ipa/rkisp1/algorithms/cproc.cpp | 4 ++-- src/ipa/rkisp1/algorithms/dpcc.cpp | 2 +- src/ipa/rkisp1/algorithms/filter.cpp | 4 ++-- src/ipa/rkisp1/algorithms/gsl.cpp | 2 +- src/ipa/rkisp1/algorithms/lsc.cpp | 2 +- src/ipa/rkisp1/ipa_context.h | 7 ++++-- src/ipa/rkisp1/rkisp1.cpp | 12 +++++----- 10 files changed, 46 insertions(+), 43 deletions(-) diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index a1bb7d97..483e941f 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -73,8 +73,8 @@ Agc::Agc() int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) { /* Configure the default exposure and gain. */ - context.frameContext.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain); - context.frameContext.agc.exposure = 10ms / context.configuration.sensor.lineDuration; + context.activeState.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain); + context.activeState.agc.exposure = 10ms / context.configuration.sensor.lineDuration; /* * According to the RkISP1 documentation: @@ -98,7 +98,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4; context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4; - /* \todo Use actual frame index by populating it in the frameContext. */ + /* \todo Use actual frame index by populating it in the activeState. */ frameCount_ = 0; return 0; } @@ -140,18 +140,18 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue) /** * \brief Estimate the new exposure and gain values - * \param[inout] frameContext The shared IPA frame Context + * \param[inout] context The shared IPA Context * \param[in] yGain The gain calculated on the current brightness level * \param[in] iqMeanGain The gain calculated based on the relative luminance target */ void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain) { IPASessionConfiguration &configuration = context.configuration; - IPAFrameContext &frameContext = context.frameContext; + IPAActiveState &activeState = context.activeState; /* Get the effective exposure and gain applied on the sensor. */ - uint32_t exposure = frameContext.sensor.exposure; - double analogueGain = frameContext.sensor.gain; + uint32_t exposure = activeState.sensor.exposure; + double analogueGain = activeState.sensor.gain; /* Use the highest of the two gain estimates. */ double evGain = std::max(yGain, iqMeanGain); @@ -216,8 +216,8 @@ void Agc::computeExposure(IPAContext &context, double yGain, double iqMeanGain) << stepGain; /* Update the estimated exposure and gain. */ - frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration; - frameContext.agc.gain = stepGain; + activeState.agc.exposure = shutterTime / configuration.sensor.lineDuration; + activeState.agc.gain = stepGain; } /** @@ -324,7 +324,7 @@ void Agc::process(IPAContext &context, */ void Agc::prepare(IPAContext &context, rkisp1_params_cfg *params) { - if (context.frameContext.frameCount > 0) + if (context.activeState.frameCount > 0) return; /* Configure the measurement window. */ diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 9f00364d..427aaeb1 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -35,9 +35,9 @@ LOG_DEFINE_CATEGORY(RkISP1Awb) int Awb::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) { - context.frameContext.awb.gains.red = 1.0; - context.frameContext.awb.gains.blue = 1.0; - context.frameContext.awb.gains.green = 1.0; + context.activeState.awb.gains.red = 1.0; + context.activeState.awb.gains.blue = 1.0; + context.activeState.awb.gains.green = 1.0; /* * Define the measurement window for AWB as a centered rectangle @@ -72,16 +72,16 @@ uint32_t Awb::estimateCCT(double red, double green, double blue) */ void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params) { - params->others.awb_gain_config.gain_green_b = 256 * context.frameContext.awb.gains.green; - params->others.awb_gain_config.gain_blue = 256 * context.frameContext.awb.gains.blue; - params->others.awb_gain_config.gain_red = 256 * context.frameContext.awb.gains.red; - params->others.awb_gain_config.gain_green_r = 256 * context.frameContext.awb.gains.green; + params->others.awb_gain_config.gain_green_b = 256 * context.activeState.awb.gains.green; + params->others.awb_gain_config.gain_blue = 256 * context.activeState.awb.gains.blue; + params->others.awb_gain_config.gain_red = 256 * context.activeState.awb.gains.red; + params->others.awb_gain_config.gain_green_r = 256 * context.activeState.awb.gains.green; /* Update the gains. */ params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_AWB_GAIN; /* If we already have configured the gains and window, return. */ - if (context.frameContext.frameCount > 0) + if (context.activeState.frameCount > 0) return; /* Configure the gains to apply. */ @@ -125,7 +125,7 @@ void Awb::process([[maybe_unused]] IPAContext &context, { const rkisp1_cif_isp_stat *params = &stats->params; const rkisp1_cif_isp_awb_stat *awb = ¶ms->awb; - IPAFrameContext &frameContext = context.frameContext; + IPAActiveState &activeState = context.activeState; /* Get the YCbCr mean values */ double yMean = awb->awb_mean[0].mean_y_or_g; @@ -157,22 +157,22 @@ void Awb::process([[maybe_unused]] IPAContext &context, /* Filter the values to avoid oscillations. */ double speed = 0.2; - redGain = speed * redGain + (1 - speed) * frameContext.awb.gains.red; - blueGain = speed * blueGain + (1 - speed) * frameContext.awb.gains.blue; + redGain = speed * redGain + (1 - speed) * activeState.awb.gains.red; + blueGain = speed * blueGain + (1 - speed) * activeState.awb.gains.blue; /* * Gain values are unsigned integer value, range 0 to 4 with 8 bit * fractional part. */ - frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256); - frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256); + activeState.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256); + activeState.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256); /* Hardcode the green gain to 1.0. */ - frameContext.awb.gains.green = 1.0; + activeState.awb.gains.green = 1.0; - frameContext.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); + activeState.awb.temperatureK = estimateCCT(redMean, greenMean, blueMean); - LOG(RkISP1Awb, Debug) << "Gain found for red: " << context.frameContext.awb.gains.red - << " and for blue: " << context.frameContext.awb.gains.blue; + LOG(RkISP1Awb, Debug) << "Gain found for red: " << context.activeState.awb.gains.red + << " and for blue: " << context.activeState.awb.gains.blue; } REGISTER_IPA_ALGORITHM(Awb, "Awb") diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp index a58569fa..4d55a2d5 100644 --- a/src/ipa/rkisp1/algorithms/blc.cpp +++ b/src/ipa/rkisp1/algorithms/blc.cpp @@ -68,7 +68,7 @@ int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context, void BlackLevelCorrection::prepare(IPAContext &context, rkisp1_params_cfg *params) { - if (context.frameContext.frameCount > 0) + if (context.activeState.frameCount > 0) return; if (!tuningParameters_) diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp index bca5ab69..a3b778d1 100644 --- a/src/ipa/rkisp1/algorithms/cproc.cpp +++ b/src/ipa/rkisp1/algorithms/cproc.cpp @@ -40,7 +40,7 @@ void ColorProcessing::queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, const ControlList &controls) { - auto &cproc = context.frameContext.cproc; + auto &cproc = context.activeState.cproc; const auto &brightness = controls.get(controls::Brightness); if (brightness) { @@ -73,7 +73,7 @@ void ColorProcessing::queueRequest(IPAContext &context, void ColorProcessing::prepare(IPAContext &context, rkisp1_params_cfg *params) { - auto &cproc = context.frameContext.cproc; + auto &cproc = context.activeState.cproc; /* Check if the algorithm configuration has been updated. */ if (!cproc.updateParams) diff --git a/src/ipa/rkisp1/algorithms/dpcc.cpp b/src/ipa/rkisp1/algorithms/dpcc.cpp index 69bc651e..93d37b1d 100644 --- a/src/ipa/rkisp1/algorithms/dpcc.cpp +++ b/src/ipa/rkisp1/algorithms/dpcc.cpp @@ -234,7 +234,7 @@ int DefectPixelClusterCorrection::init([[maybe_unused]] IPAContext &context, void DefectPixelClusterCorrection::prepare(IPAContext &context, rkisp1_params_cfg *params) { - if (context.frameContext.frameCount > 0) + if (context.activeState.frameCount > 0) return; if (!initialized_) diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp index 8ca10fd1..bc7fc1f3 100644 --- a/src/ipa/rkisp1/algorithms/filter.cpp +++ b/src/ipa/rkisp1/algorithms/filter.cpp @@ -46,7 +46,7 @@ void Filter::queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, const ControlList &controls) { - auto &filter = context.frameContext.filter; + auto &filter = context.activeState.filter; const auto &sharpness = controls.get(controls::Sharpness); if (sharpness) { @@ -87,7 +87,7 @@ void Filter::queueRequest(IPAContext &context, */ void Filter::prepare(IPAContext &context, rkisp1_params_cfg *params) { - auto &filter = context.frameContext.filter; + auto &filter = context.activeState.filter; /* Check if the algorithm configuration has been updated. */ if (!filter.updateParams) diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp index 2fd1a23d..dd997462 100644 --- a/src/ipa/rkisp1/algorithms/gsl.cpp +++ b/src/ipa/rkisp1/algorithms/gsl.cpp @@ -121,7 +121,7 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context, void GammaSensorLinearization::prepare(IPAContext &context, rkisp1_params_cfg *params) { - if (context.frameContext.frameCount > 0) + if (context.activeState.frameCount > 0) return; if (!initialized_) diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp index 05c8c0da..4ed46708 100644 --- a/src/ipa/rkisp1/algorithms/lsc.cpp +++ b/src/ipa/rkisp1/algorithms/lsc.cpp @@ -125,7 +125,7 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context, void LensShadingCorrection::prepare(IPAContext &context, rkisp1_params_cfg *params) { - if (context.frameContext.frameCount > 0) + if (context.activeState.frameCount > 0) return; if (!initialized_) diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index 2bdb6a81..a8daeca4 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -41,7 +41,7 @@ struct IPASessionConfiguration { } hw; }; -struct IPAFrameContext { +struct IPAActiveState { struct { uint32_t exposure; double gain; @@ -78,9 +78,12 @@ struct IPAFrameContext { unsigned int frameCount; }; +struct IPAFrameContext { +}; + struct IPAContext { IPASessionConfiguration configuration; - IPAFrameContext frameContext; + IPAActiveState activeState; }; } /* namespace ipa::rkisp1 */ diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 0873d7fe..5531f55b 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -255,7 +255,7 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); - context_.frameContext.frameCount = 0; + context_.activeState.frameCount = 0; for (auto const &algo : algorithms()) { int ret = algo->configure(context_, info); @@ -315,7 +315,7 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) algo->prepare(context_, params); paramsBufferReady.emit(frame); - context_.frameContext.frameCount++; + context_.activeState.frameCount++; } void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId, @@ -325,9 +325,9 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId reinterpret_cast( mappedBuffers_.at(bufferId).planes()[0].data()); - context_.frameContext.sensor.exposure = + context_.activeState.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get(); - context_.frameContext.sensor.gain = + context_.activeState.sensor.gain = camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get()); unsigned int aeState = 0; @@ -342,8 +342,8 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId void IPARkISP1::setControls(unsigned int frame) { - uint32_t exposure = context_.frameContext.agc.exposure; - uint32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain); + uint32_t exposure = context_.activeState.agc.exposure; + uint32_t gain = camHelper_->gainCode(context_.activeState.agc.gain); ControlList ctrls(ctrls_); ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure)); From 6903b6311e4e570ae0590c994f6623ddc8cebb06 Mon Sep 17 00:00:00 2001 From: Kieran Bingham via libcamera-devel Date: Thu, 21 Jul 2022 13:13:04 +0100 Subject: [PATCH 11/15] ipa: libipa: Provide a common base for FrameContexts Provide a common IPAFrameContext as a base for IPA modules to inherit from. This will allow having a common set of parameters for every FrameContext. We still have to use a templated instance of the FCQueue though to make sure we allocate the correct types. Both the RKISP1 and the IPU3 create IPA specific FrameContext structures which inherit from the IPAFrameContext. While adjusting interface to Algorithm::process() the FrameContext is converted from a pointer to a reference. Signed-off-by: Kieran Bingham Signed-off-by: Jacopo Mondi --- src/ipa/ipu3/algorithms/af.cpp | 3 ++- src/ipa/ipu3/algorithms/af.h | 2 +- src/ipa/ipu3/algorithms/agc.cpp | 9 ++++---- src/ipa/ipu3/algorithms/agc.h | 4 ++-- src/ipa/ipu3/algorithms/awb.cpp | 3 ++- src/ipa/ipu3/algorithms/awb.h | 2 +- src/ipa/ipu3/algorithms/tone_mapping.cpp | 3 ++- src/ipa/ipu3/algorithms/tone_mapping.h | 2 +- src/ipa/ipu3/ipa_context.cpp | 29 ++++-------------------- src/ipa/ipu3/ipa_context.h | 8 ++----- src/ipa/ipu3/ipu3.cpp | 6 ++--- src/ipa/ipu3/module.h | 2 +- src/ipa/libipa/algorithm.h | 2 +- src/ipa/libipa/fc_queue.cpp | 21 +++++++++++++++++ src/ipa/libipa/fc_queue.h | 5 ++++ src/ipa/rkisp1/algorithms/agc.cpp | 2 +- src/ipa/rkisp1/algorithms/agc.h | 2 +- src/ipa/rkisp1/algorithms/awb.cpp | 2 +- src/ipa/rkisp1/algorithms/awb.h | 2 +- src/ipa/rkisp1/ipa_context.h | 4 +++- src/ipa/rkisp1/module.h | 2 +- src/ipa/rkisp1/rkisp1.cpp | 5 +++- 22 files changed, 66 insertions(+), 54 deletions(-) diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp index d07521a0..fcbf8e55 100644 --- a/src/ipa/ipu3/algorithms/af.cpp +++ b/src/ipa/ipu3/algorithms/af.cpp @@ -420,7 +420,8 @@ bool Af::afIsOutOfFocus(IPAContext context) * * [1] Hill Climbing Algorithm, https://en.wikipedia.org/wiki/Hill_climbing */ -void Af::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameContext, +void Af::process(IPAContext &context, + [[maybe_unused]] IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) { /* Evaluate the AF buffer length */ diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h index ccf015f3..7a5aeb1b 100644 --- a/src/ipa/ipu3/algorithms/af.h +++ b/src/ipa/ipu3/algorithms/af.h @@ -32,7 +32,7 @@ class Af : public Algorithm void prepare(IPAContext &context, ipu3_uapi_params *params) override; int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; - void process(IPAContext &context, IPAFrameContext *frameContext, + void process(IPAContext &context, IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; private: diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 5bc64ae5..92ea6249 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -183,13 +183,13 @@ utils::Duration Agc::filterExposure(utils::Duration exposureValue) * \param[in] yGain The gain calculated based on the relative luminance target * \param[in] iqMeanGain The gain calculated based on the relative luminance target */ -void Agc::computeExposure(IPAContext &context, IPAFrameContext *frameContext, +void Agc::computeExposure(IPAContext &context, IPU3FrameContext &frameContext, double yGain, double iqMeanGain) { const IPASessionConfiguration &configuration = context.configuration; /* Get the effective exposure and gain applied on the sensor. */ - uint32_t exposure = frameContext->sensor.exposure; - double analogueGain = frameContext->sensor.gain; + uint32_t exposure = frameContext.sensor.exposure; + double analogueGain = frameContext.sensor.gain; /* Use the highest of the two gain estimates. */ double evGain = std::max(yGain, iqMeanGain); @@ -323,7 +323,8 @@ double Agc::estimateLuminance(IPAActiveState &activeState, * Identify the current image brightness, and use that to estimate the optimal * new exposure and gain for the scene. */ -void Agc::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameContext, +void Agc::process(IPAContext &context, + IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) { /* diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h index 105ae0f2..96585f48 100644 --- a/src/ipa/ipu3/algorithms/agc.h +++ b/src/ipa/ipu3/algorithms/agc.h @@ -28,14 +28,14 @@ class Agc : public Algorithm ~Agc() = default; int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; - void process(IPAContext &context, IPAFrameContext *frameContext, + void process(IPAContext &context, IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; private: double measureBrightness(const ipu3_uapi_stats_3a *stats, const ipu3_uapi_grid_config &grid) const; utils::Duration filterExposure(utils::Duration currentExposure); - void computeExposure(IPAContext &context, IPAFrameContext *frameContext, + void computeExposure(IPAContext &context, IPU3FrameContext &frameContext, double yGain, double iqMeanGain); double estimateLuminance(IPAActiveState &activeState, const ipu3_uapi_grid_config &grid, diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index 70426722..45f8e5f2 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -387,7 +387,8 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats) /** * \copydoc libcamera::ipa::Algorithm::process */ -void Awb::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameContext, +void Awb::process(IPAContext &context, + [[maybe_unused]] IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) { calculateWBGains(stats); diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h index 0acd2148..28e39620 100644 --- a/src/ipa/ipu3/algorithms/awb.h +++ b/src/ipa/ipu3/algorithms/awb.h @@ -40,7 +40,7 @@ class Awb : public Algorithm int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; void prepare(IPAContext &context, ipu3_uapi_params *params) override; - void process(IPAContext &context, IPAFrameContext *frameContext, + void process(IPAContext &context, IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; private: diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp index f86e79b2..977741c5 100644 --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp @@ -78,7 +78,8 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context, * The tone mapping look up table is generated as an inverse power curve from * our gamma setting. */ -void ToneMapping::process(IPAContext &context, [[maybe_unused]] IPAFrameContext *frameContext, +void ToneMapping::process(IPAContext &context, + [[maybe_unused]] IPU3FrameContext &frameContext, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) { /* diff --git a/src/ipa/ipu3/algorithms/tone_mapping.h b/src/ipa/ipu3/algorithms/tone_mapping.h index d7d48006..8cce38c7 100644 --- a/src/ipa/ipu3/algorithms/tone_mapping.h +++ b/src/ipa/ipu3/algorithms/tone_mapping.h @@ -20,7 +20,7 @@ class ToneMapping : public Algorithm int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; void prepare(IPAContext &context, ipu3_uapi_params *params) override; - void process(IPAContext &context, IPAFrameContext *frameContext, + void process(IPAContext &context, IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; private: diff --git a/src/ipa/ipu3/ipa_context.cpp b/src/ipa/ipu3/ipa_context.cpp index 9605c8a1..536d76ec 100644 --- a/src/ipa/ipu3/ipa_context.cpp +++ b/src/ipa/ipu3/ipa_context.cpp @@ -35,22 +35,6 @@ namespace libcamera::ipa::ipu3 { * most recently computed by the IPA algorithms. */ -/** - * \struct IPAFrameContext - * \brief Context for a frame - * - * The frame context stores data specific to a single frame processed by the - * IPA. Each frame processed by the IPA has a context associated with it, - * accessible through the IPAContext structure. - * - * Fields in the frame context should reflect values and controls - * associated with the specific frame as requested by the application, and - * as configured by the hardware. Fields can be read by algorithms to - * determine if they should update any specific action for this frame, and - * finally to update the metadata control lists when the frame is fully - * completed. - */ - /** * \struct IPAContext * \brief Global IPA context data shared between all algorithms @@ -181,20 +165,17 @@ namespace libcamera::ipa::ipu3 { */ /** - * \var IPAFrameContext::frame - * \brief The frame number + * \struct IPU3FrameContext + * \copybrief libcamera::ipa::IPAFrameContext * - * \var IPAFrameContext::sensor + * \var IPU3FrameContext::sensor * \brief Effective sensor values that were applied for the frame * - * \var IPAFrameContext::sensor.exposure + * \var IPU3FrameContext::sensor.exposure * \brief Exposure time expressed as a number of lines * - * \var IPAFrameContext::sensor.gain + * \var IPU3FrameContext::sensor.gain * \brief Analogue gain multiplier - * - * \var IPAFrameContext::error - * \brief The error flags for this frame context */ } /* namespace libcamera::ipa::ipu3 */ diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h index dc5b7c30..c4aa4c3f 100644 --- a/src/ipa/ipu3/ipa_context.h +++ b/src/ipa/ipu3/ipa_context.h @@ -14,7 +14,6 @@ #include #include -#include #include @@ -74,22 +73,19 @@ struct IPAActiveState { } toneMapping; }; -struct IPAFrameContext { - uint32_t frame; +struct IPU3FrameContext : public IPAFrameContext { struct { uint32_t exposure; double gain; } sensor; - - Request::Errors error; }; struct IPAContext { IPASessionConfiguration configuration; IPAActiveState activeState; - FCQueue frameContexts; + FCQueue frameContexts; }; } /* namespace ipa::ipu3 */ diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 209a6b04..24839fd1 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -569,7 +569,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, const ipu3_uapi_stats_3a *stats = reinterpret_cast(mem.data()); - IPAFrameContext &frameContext = context_.frameContexts.get(frame); + IPU3FrameContext &frameContext = context_.frameContexts.get(frame); if (frameContext.frame != frame) LOG(IPAIPU3, Warning) << "Frame " << frame << " does not match its frame context"; @@ -582,7 +582,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, ControlList ctrls(controls::controls); for (auto const &algo : algorithms_) - algo->process(context_, &frameContext, stats); + algo->process(context_, frameContext, stats); setControls(frame); @@ -618,7 +618,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) { /* \todo Start processing for 'frame' based on 'controls'. */ - IPAFrameContext &frameContext = context_.frameContexts.initialise(frame); + IPU3FrameContext &frameContext = context_.frameContexts.initialise(frame); /* \todo Implement queueRequest to each algorithm. */ (void)frameContext; diff --git a/src/ipa/ipu3/module.h b/src/ipa/ipu3/module.h index d94fc459..6d0d50f6 100644 --- a/src/ipa/ipu3/module.h +++ b/src/ipa/ipu3/module.h @@ -19,7 +19,7 @@ namespace libcamera { namespace ipa::ipu3 { -using Module = ipa::Module; } /* namespace ipa::ipu3 */ diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h index ccc659a6..0fe3d772 100644 --- a/src/ipa/libipa/algorithm.h +++ b/src/ipa/libipa/algorithm.h @@ -49,7 +49,7 @@ class Algorithm } virtual void process([[maybe_unused]] typename Module::Context &context, - [[maybe_unused]] typename Module::FrameContext *frameContext, + [[maybe_unused]] typename Module::FrameContext &frameContext, [[maybe_unused]] const typename Module::Stats *stats) { } diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp index 37ffca60..f0d0b3c7 100644 --- a/src/ipa/libipa/fc_queue.cpp +++ b/src/ipa/libipa/fc_queue.cpp @@ -66,6 +66,27 @@ namespace ipa { * the PFCError flag is automatically raised on the FrameContext. */ +/** + * \struct IPAFrameContext + * \brief Context for a frame + * + * The frame context stores data specific to a single frame processed by the + * IPA. Each frame processed by the IPA has a context associated with it, + * accessible through the Frame Context Queue. + * + * Fields in the frame context should reflect values and controls associated + * with the specific frame as requested by the application, and as configured by + * the hardware. Fields can be read by algorithms to determine if they should + * update any specific action for this frame, and finally to update the metadata + * control lists when the frame is fully completed. + * + * \var IPAFrameContext::frame + * \brief The frame number + * + * \var IPAFrameContext::error + * \brief The error flags associated with the frame + */ + /** * \fn FCQueue::clear() * \brief Clear the context queue diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h index c385d42e..28b58e4b 100644 --- a/src/ipa/libipa/fc_queue.h +++ b/src/ipa/libipa/fc_queue.h @@ -26,6 +26,11 @@ namespace ipa { */ static constexpr uint32_t kMaxFrameContexts = 16; +struct IPAFrameContext { + unsigned int frame; + Request::Errors error; +}; + template class FCQueue : private std::array { diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 483e941f..40d27963 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -281,7 +281,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const * new exposure and gain for the scene. */ void Agc::process(IPAContext &context, - [[maybe_unused]] IPAFrameContext *frameContext, + [[maybe_unused]] RKISP1FrameContext &frameContext, const rkisp1_stat_buffer *stats) { const rkisp1_cif_isp_stat *params = &stats->params; diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h index 1c9818b7..af7fe274 100644 --- a/src/ipa/rkisp1/algorithms/agc.h +++ b/src/ipa/rkisp1/algorithms/agc.h @@ -27,7 +27,7 @@ class Agc : public Algorithm int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; void prepare(IPAContext &context, rkisp1_params_cfg *params) override; - void process(IPAContext &context, IPAFrameContext *frameContext, + void process(IPAContext &context, RKISP1FrameContext &frameContext, const rkisp1_stat_buffer *stats) override; private: diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 427aaeb1..3beafb73 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -120,7 +120,7 @@ void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params) * \copydoc libcamera::ipa::Algorithm::process */ void Awb::process([[maybe_unused]] IPAContext &context, - [[maybe_unused]] IPAFrameContext *frameCtx, + [[maybe_unused]] RKISP1FrameContext &frameCtx, const rkisp1_stat_buffer *stats) { const rkisp1_cif_isp_stat *params = &stats->params; diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h index 667a8beb..5954034b 100644 --- a/src/ipa/rkisp1/algorithms/awb.h +++ b/src/ipa/rkisp1/algorithms/awb.h @@ -21,7 +21,7 @@ class Awb : public Algorithm int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; void prepare(IPAContext &context, rkisp1_params_cfg *params) override; - void process(IPAContext &context, IPAFrameContext *frameCtx, + void process(IPAContext &context, RKISP1FrameContext &frameCtx, const rkisp1_stat_buffer *stats) override; private: diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index a8daeca4..c42bcd73 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -14,6 +14,8 @@ #include +#include + namespace libcamera { namespace ipa::rkisp1 { @@ -78,7 +80,7 @@ struct IPAActiveState { unsigned int frameCount; }; -struct IPAFrameContext { +struct RKISP1FrameContext : public IPAFrameContext { }; struct IPAContext { diff --git a/src/ipa/rkisp1/module.h b/src/ipa/rkisp1/module.h index 89f83208..2568c624 100644 --- a/src/ipa/rkisp1/module.h +++ b/src/ipa/rkisp1/module.h @@ -19,7 +19,7 @@ namespace libcamera { namespace ipa::rkisp1 { -using Module = ipa::Module; } /* namespace ipa::rkisp1 */ diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 5531f55b..30a1c453 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -332,8 +332,11 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId unsigned int aeState = 0; + /* \todo Obtain the frame context to pass to process from the FCQueue */ + RKISP1FrameContext frameContext; + for (auto const &algo : algorithms()) - algo->process(context_, nullptr, stats); + algo->process(context_, frameContext, stats); setControls(frame); From 97d07ed49816c482c29fb2997cc42728c61d8c27 Mon Sep 17 00:00:00 2001 From: Kieran Bingham via libcamera-devel Date: Thu, 21 Jul 2022 13:13:05 +0100 Subject: [PATCH 12/15] ipa: rkisp1: Convert to use the FCQueue Establish a queue of FrameContexts using the new FCQueue and use it to supply the FrameContext to the algorithms. The algorithms on the RKISP1 do not use this yet themselves, but are able to do so after the introduction of this patch. Signed-off-by: Kieran Bingham Signed-off-by: Jacopo Mondi --- src/ipa/rkisp1/ipa_context.h | 2 ++ src/ipa/rkisp1/rkisp1.cpp | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index c42bcd73..dd756f40 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -86,6 +86,8 @@ struct RKISP1FrameContext : public IPAFrameContext { struct IPAContext { IPASessionConfiguration configuration; IPAActiveState activeState; + + FCQueue frameContexts; }; } /* namespace ipa::rkisp1 */ diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 30a1c453..08324730 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -195,6 +195,7 @@ int IPARkISP1::start() void IPARkISP1::stop() { /* Clean the IPA context at the end of the streaming session. */ + context_.frameContexts.clear(); context_ = {}; } @@ -321,6 +322,8 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId, const ControlList &sensorControls) { + RKISP1FrameContext &frameContext = context_.frameContexts.get(frame); + const rkisp1_stat_buffer *stats = reinterpret_cast( mappedBuffers_.at(bufferId).planes()[0].data()); @@ -332,9 +335,6 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId unsigned int aeState = 0; - /* \todo Obtain the frame context to pass to process from the FCQueue */ - RKISP1FrameContext frameContext; - for (auto const &algo : algorithms()) algo->process(context_, frameContext, stats); From a1ff48b8fd6f89689e76f7e0e12523f10dbb1fa5 Mon Sep 17 00:00:00 2001 From: Kieran Bingham via libcamera-devel Date: Thu, 21 Jul 2022 13:13:06 +0100 Subject: [PATCH 13/15] ipa: libipa: algorithm: prepare(): Pass frame and frame Context Pass the current frame number, and the current FrameContext for calls to prepare. Until there is a Frame Context Queue in the RKISP1 the FrameContext will be a nullptr, and must not be dereferenced until the queue is implemented. Signed-off-by: Kieran Bingham - Obtain and use the RKISP1FrameContext Queue entry for IPARkISP1::fillParamsBuffer Signed-off-by: Kieran Bingham Signed-off-by: Jacopo Mondi --- src/ipa/ipu3/algorithms/af.cpp | 5 ++++- src/ipa/ipu3/algorithms/af.h | 4 +++- src/ipa/ipu3/algorithms/awb.cpp | 5 ++++- src/ipa/ipu3/algorithms/awb.h | 5 ++++- src/ipa/ipu3/algorithms/blc.cpp | 6 +++++- src/ipa/ipu3/algorithms/blc.h | 4 +++- src/ipa/ipu3/algorithms/tone_mapping.cpp | 4 ++++ src/ipa/ipu3/algorithms/tone_mapping.h | 3 ++- src/ipa/ipu3/ipu3.cpp | 4 +++- src/ipa/libipa/algorithm.cpp | 2 ++ src/ipa/libipa/algorithm.h | 2 ++ src/ipa/rkisp1/algorithms/agc.cpp | 5 ++++- src/ipa/rkisp1/algorithms/agc.h | 4 +++- src/ipa/rkisp1/algorithms/awb.cpp | 5 ++++- src/ipa/rkisp1/algorithms/awb.h | 4 +++- src/ipa/rkisp1/algorithms/blc.cpp | 2 ++ src/ipa/rkisp1/algorithms/blc.h | 4 +++- src/ipa/rkisp1/algorithms/cproc.cpp | 2 ++ src/ipa/rkisp1/algorithms/cproc.h | 4 +++- src/ipa/rkisp1/algorithms/dpcc.cpp | 2 ++ src/ipa/rkisp1/algorithms/dpcc.h | 4 +++- src/ipa/rkisp1/algorithms/filter.cpp | 5 ++++- src/ipa/rkisp1/algorithms/filter.h | 4 +++- src/ipa/rkisp1/algorithms/gsl.cpp | 2 ++ src/ipa/rkisp1/algorithms/gsl.h | 4 +++- src/ipa/rkisp1/algorithms/lsc.cpp | 2 ++ src/ipa/rkisp1/algorithms/lsc.h | 4 +++- src/ipa/rkisp1/rkisp1.cpp | 4 +++- 28 files changed, 85 insertions(+), 20 deletions(-) diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp index fcbf8e55..1faf9296 100644 --- a/src/ipa/ipu3/algorithms/af.cpp +++ b/src/ipa/ipu3/algorithms/af.cpp @@ -116,7 +116,10 @@ Af::Af() /** * \copydoc libcamera::ipa::Algorithm::prepare */ -void Af::prepare(IPAContext &context, ipu3_uapi_params *params) +void Af::prepare(IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] IPU3FrameContext &frameContext, + ipu3_uapi_params *params) { const struct ipu3_uapi_grid_config &grid = context.configuration.af.afGrid; params->acc_param.af.grid_cfg = grid; diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h index 7a5aeb1b..4e2bbd08 100644 --- a/src/ipa/ipu3/algorithms/af.h +++ b/src/ipa/ipu3/algorithms/af.h @@ -30,8 +30,10 @@ class Af : public Algorithm Af(); ~Af() = default; - void prepare(IPAContext &context, ipu3_uapi_params *params) override; int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; + void prepare(IPAContext &context, unsigned int frame, + IPU3FrameContext &frameContext, + ipu3_uapi_params *params) override; void process(IPAContext &context, IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index 45f8e5f2..b110ad40 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -430,7 +430,10 @@ constexpr uint16_t Awb::gainValue(double gain) /** * \copydoc libcamera::ipa::Algorithm::prepare */ -void Awb::prepare(IPAContext &context, ipu3_uapi_params *params) +void Awb::prepare(IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] IPU3FrameContext &frameContext, + ipu3_uapi_params *params) { /* * Green saturation thresholds are reduced because we are using the diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h index 28e39620..731e5bae 100644 --- a/src/ipa/ipu3/algorithms/awb.h +++ b/src/ipa/ipu3/algorithms/awb.h @@ -39,7 +39,10 @@ class Awb : public Algorithm ~Awb(); int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; - void prepare(IPAContext &context, ipu3_uapi_params *params) override; + + void prepare(IPAContext &context, unsigned int frame, + IPU3FrameContext &frameContext, + ipu3_uapi_params *params) override; void process(IPAContext &context, IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; diff --git a/src/ipa/ipu3/algorithms/blc.cpp b/src/ipa/ipu3/algorithms/blc.cpp index 78ab7bff..7b0d720a 100644 --- a/src/ipa/ipu3/algorithms/blc.cpp +++ b/src/ipa/ipu3/algorithms/blc.cpp @@ -39,13 +39,17 @@ BlackLevelCorrection::BlackLevelCorrection() /** * \brief Fill in the parameter structure, and enable black level correction * \param context The shared IPA context + * \param[in] frame The frame context sequence number + * \param[in] frameContext The FrameContext for this frame * \param params The IPU3 parameters * * Populate the IPU3 parameter structure with the correction values for each * channel and enable the corresponding ImgU block processing. */ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context, - ipu3_uapi_params *params) + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] IPU3FrameContext &frameContext, + ipu3_uapi_params *params) { /* * The Optical Black Level correction values diff --git a/src/ipa/ipu3/algorithms/blc.h b/src/ipa/ipu3/algorithms/blc.h index d8da1748..36388b26 100644 --- a/src/ipa/ipu3/algorithms/blc.h +++ b/src/ipa/ipu3/algorithms/blc.h @@ -18,7 +18,9 @@ class BlackLevelCorrection : public Algorithm public: BlackLevelCorrection(); - void prepare(IPAContext &context, ipu3_uapi_params *params) override; + void prepare(IPAContext &context, unsigned int frame, + IPU3FrameContext &frameContext, + ipu3_uapi_params *params) override; }; } /* namespace ipa::ipu3::algorithms */ diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp index 977741c5..0951db43 100644 --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp @@ -50,12 +50,16 @@ int ToneMapping::configure(IPAContext &context, /** * \brief Fill in the parameter structure, and enable gamma control * \param context The shared IPA context + * \param[in] frame The frame context sequence number + * \param[in] frameContext The FrameContext for this frame * \param params The IPU3 parameters * * Populate the IPU3 parameter structure with our tone mapping look up table and * enable the gamma control module in the processing blocks. */ void ToneMapping::prepare([[maybe_unused]] IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] IPU3FrameContext &frameContext, ipu3_uapi_params *params) { /* Copy the calculated LUT into the parameters buffer. */ diff --git a/src/ipa/ipu3/algorithms/tone_mapping.h b/src/ipa/ipu3/algorithms/tone_mapping.h index 8cce38c7..cfb3de01 100644 --- a/src/ipa/ipu3/algorithms/tone_mapping.h +++ b/src/ipa/ipu3/algorithms/tone_mapping.h @@ -19,7 +19,8 @@ class ToneMapping : public Algorithm ToneMapping(); int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; - void prepare(IPAContext &context, ipu3_uapi_params *params) override; + void prepare(IPAContext &context, unsigned int frame, + IPU3FrameContext &frameContext, ipu3_uapi_params *params) override; void process(IPAContext &context, IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 24839fd1..ffb9ea8c 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -538,8 +538,10 @@ void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) */ params->use = {}; + IPU3FrameContext frameContext = context_.frameContexts.get(frame); + for (auto const &algo : algorithms_) - algo->prepare(context_, params); + algo->prepare(context_, frame, frameContext, params); paramsBufferReady.emit(frame); } diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp index 38200e57..0992e5de 100644 --- a/src/ipa/libipa/algorithm.cpp +++ b/src/ipa/libipa/algorithm.cpp @@ -70,6 +70,8 @@ namespace ipa { * \fn Algorithm::prepare() * \brief Fill the \a params buffer with ISP processing parameters for a frame * \param[in] context The shared IPA context + * \param[in] frame The frame context sequence number + * \param[in] frameContext The FrameContext for this frame * \param[out] params The ISP specific parameters. * * This function is called for every frame when the camera is running before it diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h index 0fe3d772..379207f1 100644 --- a/src/ipa/libipa/algorithm.h +++ b/src/ipa/libipa/algorithm.h @@ -38,6 +38,8 @@ class Algorithm } virtual void prepare([[maybe_unused]] typename Module::Context &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] typename Module::FrameContext &frameContext, [[maybe_unused]] typename Module::Params *params) { } diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 40d27963..2d436511 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -322,7 +322,10 @@ void Agc::process(IPAContext &context, /** * \copydoc libcamera::ipa::Algorithm::prepare */ -void Agc::prepare(IPAContext &context, rkisp1_params_cfg *params) +void Agc::prepare(IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) { if (context.activeState.frameCount > 0) return; diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h index af7fe274..97ca0e31 100644 --- a/src/ipa/rkisp1/algorithms/agc.h +++ b/src/ipa/rkisp1/algorithms/agc.h @@ -26,7 +26,9 @@ class Agc : public Algorithm ~Agc() = default; int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; - void prepare(IPAContext &context, rkisp1_params_cfg *params) override; + void prepare(IPAContext &context, unsigned int frame, + RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) override; void process(IPAContext &context, RKISP1FrameContext &frameContext, const rkisp1_stat_buffer *stats) override; diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 3beafb73..1af6d98c 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -70,7 +70,10 @@ uint32_t Awb::estimateCCT(double red, double green, double blue) /** * \copydoc libcamera::ipa::Algorithm::prepare */ -void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params) +void Awb::prepare(IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) { params->others.awb_gain_config.gain_green_b = 256 * context.activeState.awb.gains.green; params->others.awb_gain_config.gain_blue = 256 * context.activeState.awb.gains.blue; diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h index 5954034b..82d5ce1b 100644 --- a/src/ipa/rkisp1/algorithms/awb.h +++ b/src/ipa/rkisp1/algorithms/awb.h @@ -20,7 +20,9 @@ class Awb : public Algorithm ~Awb() = default; int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; - void prepare(IPAContext &context, rkisp1_params_cfg *params) override; + void prepare(IPAContext &context, unsigned int frame, + RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) override; void process(IPAContext &context, RKISP1FrameContext &frameCtx, const rkisp1_stat_buffer *stats) override; diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp index 4d55a2d5..93c7e314 100644 --- a/src/ipa/rkisp1/algorithms/blc.cpp +++ b/src/ipa/rkisp1/algorithms/blc.cpp @@ -66,6 +66,8 @@ int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context, * \copydoc libcamera::ipa::Algorithm::prepare */ void BlackLevelCorrection::prepare(IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] RKISP1FrameContext &frameContext, rkisp1_params_cfg *params) { if (context.activeState.frameCount > 0) diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h index 5fc3a80f..1afa12f6 100644 --- a/src/ipa/rkisp1/algorithms/blc.h +++ b/src/ipa/rkisp1/algorithms/blc.h @@ -20,7 +20,9 @@ class BlackLevelCorrection : public Algorithm ~BlackLevelCorrection() = default; int init(IPAContext &context, const YamlObject &tuningData) override; - void prepare(IPAContext &context, rkisp1_params_cfg *params) override; + void prepare(IPAContext &context, unsigned int frame, + RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) override; private: bool tuningParameters_; diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp index a3b778d1..edaf2d10 100644 --- a/src/ipa/rkisp1/algorithms/cproc.cpp +++ b/src/ipa/rkisp1/algorithms/cproc.cpp @@ -71,6 +71,8 @@ void ColorProcessing::queueRequest(IPAContext &context, * \copydoc libcamera::ipa::Algorithm::prepare */ void ColorProcessing::prepare(IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] RKISP1FrameContext &frameContext, rkisp1_params_cfg *params) { auto &cproc = context.activeState.cproc; diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h index 4b7e4064..fde83f3c 100644 --- a/src/ipa/rkisp1/algorithms/cproc.h +++ b/src/ipa/rkisp1/algorithms/cproc.h @@ -23,7 +23,9 @@ class ColorProcessing : public Algorithm void queueRequest(IPAContext &context, const uint32_t frame, const ControlList &controls) override; - void prepare(IPAContext &context, rkisp1_params_cfg *params) override; + void prepare(IPAContext &context, unsigned int frame, + RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) override; }; } /* namespace ipa::rkisp1::algorithms */ diff --git a/src/ipa/rkisp1/algorithms/dpcc.cpp b/src/ipa/rkisp1/algorithms/dpcc.cpp index 93d37b1d..9ba03653 100644 --- a/src/ipa/rkisp1/algorithms/dpcc.cpp +++ b/src/ipa/rkisp1/algorithms/dpcc.cpp @@ -232,6 +232,8 @@ int DefectPixelClusterCorrection::init([[maybe_unused]] IPAContext &context, * \copydoc libcamera::ipa::Algorithm::prepare */ void DefectPixelClusterCorrection::prepare(IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] RKISP1FrameContext &frameContext, rkisp1_params_cfg *params) { if (context.activeState.frameCount > 0) diff --git a/src/ipa/rkisp1/algorithms/dpcc.h b/src/ipa/rkisp1/algorithms/dpcc.h index a363f7be..ca9df9fc 100644 --- a/src/ipa/rkisp1/algorithms/dpcc.h +++ b/src/ipa/rkisp1/algorithms/dpcc.h @@ -20,7 +20,9 @@ class DefectPixelClusterCorrection : public Algorithm ~DefectPixelClusterCorrection() = default; int init(IPAContext &context, const YamlObject &tuningData) override; - void prepare(IPAContext &context, rkisp1_params_cfg *params) override; + void prepare(IPAContext &context, unsigned int frame, + RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) override; private: bool initialized_; diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp index bc7fc1f3..f6577046 100644 --- a/src/ipa/rkisp1/algorithms/filter.cpp +++ b/src/ipa/rkisp1/algorithms/filter.cpp @@ -85,7 +85,10 @@ void Filter::queueRequest(IPAContext &context, /** * \copydoc libcamera::ipa::Algorithm::prepare */ -void Filter::prepare(IPAContext &context, rkisp1_params_cfg *params) +void Filter::prepare(IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) { auto &filter = context.activeState.filter; diff --git a/src/ipa/rkisp1/algorithms/filter.h b/src/ipa/rkisp1/algorithms/filter.h index 9eb170eb..1fbe785a 100644 --- a/src/ipa/rkisp1/algorithms/filter.h +++ b/src/ipa/rkisp1/algorithms/filter.h @@ -23,7 +23,9 @@ class Filter : public Algorithm void queueRequest(IPAContext &context, const uint32_t frame, const ControlList &controls) override; - void prepare(IPAContext &context, rkisp1_params_cfg *params) override; + void prepare(IPAContext &context, unsigned int frame, + RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) override; }; } /* namespace ipa::rkisp1::algorithms */ diff --git a/src/ipa/rkisp1/algorithms/gsl.cpp b/src/ipa/rkisp1/algorithms/gsl.cpp index dd997462..77d841cf 100644 --- a/src/ipa/rkisp1/algorithms/gsl.cpp +++ b/src/ipa/rkisp1/algorithms/gsl.cpp @@ -119,6 +119,8 @@ int GammaSensorLinearization::init([[maybe_unused]] IPAContext &context, * \copydoc libcamera::ipa::Algorithm::prepare */ void GammaSensorLinearization::prepare(IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] RKISP1FrameContext &frameContext, rkisp1_params_cfg *params) { if (context.activeState.frameCount > 0) diff --git a/src/ipa/rkisp1/algorithms/gsl.h b/src/ipa/rkisp1/algorithms/gsl.h index db287dc2..fe74ae10 100644 --- a/src/ipa/rkisp1/algorithms/gsl.h +++ b/src/ipa/rkisp1/algorithms/gsl.h @@ -20,7 +20,9 @@ class GammaSensorLinearization : public Algorithm ~GammaSensorLinearization() = default; int init(IPAContext &context, const YamlObject &tuningData) override; - void prepare(IPAContext &context, rkisp1_params_cfg *params) override; + void prepare(IPAContext &context, unsigned int frame, + RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) override; private: bool initialized_; diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp index 4ed46708..cd8fcbe6 100644 --- a/src/ipa/rkisp1/algorithms/lsc.cpp +++ b/src/ipa/rkisp1/algorithms/lsc.cpp @@ -123,6 +123,8 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context, * \copydoc libcamera::ipa::Algorithm::prepare */ void LensShadingCorrection::prepare(IPAContext &context, + [[maybe_unused]] unsigned int frame, + [[maybe_unused]] RKISP1FrameContext &frameContext, rkisp1_params_cfg *params) { if (context.activeState.frameCount > 0) diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h index fdb2ec1d..fafb80b3 100644 --- a/src/ipa/rkisp1/algorithms/lsc.h +++ b/src/ipa/rkisp1/algorithms/lsc.h @@ -20,7 +20,9 @@ class LensShadingCorrection : public Algorithm ~LensShadingCorrection() = default; int init(IPAContext &context, const YamlObject &tuningData) override; - void prepare(IPAContext &context, rkisp1_params_cfg *params) override; + void prepare(IPAContext &context, unsigned int frame, + RKISP1FrameContext &frameContext, + rkisp1_params_cfg *params) override; private: bool initialized_; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 08324730..091be3fc 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -305,6 +305,8 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls) void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) { + RKISP1FrameContext &frameContext = context_.frameContexts.get(frame); + rkisp1_params_cfg *params = reinterpret_cast( mappedBuffers_.at(bufferId).planes()[0].data()); @@ -313,7 +315,7 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) memset(params, 0, sizeof(*params)); for (auto const &algo : algorithms()) - algo->prepare(context_, params); + algo->prepare(context_, frame, frameContext, params); paramsBufferReady.emit(frame); context_.activeState.frameCount++; From 234db01f70ef92a8a616b407528bf428211172d2 Mon Sep 17 00:00:00 2001 From: Kieran Bingham via libcamera-devel Date: Thu, 21 Jul 2022 13:13:07 +0100 Subject: [PATCH 14/15] ipa: libipa: algorithm: process(): Pass frame number Pass the frame number of the current frame being processed. Signed-off-by: Kieran Bingham - Use RKISP1FrameContext from the queue during IPARkISP1::processStatsBuffer Signed-off-by: Kieran Bingham --- src/ipa/ipu3/algorithms/af.cpp | 2 ++ src/ipa/ipu3/algorithms/af.h | 4 +++- src/ipa/ipu3/algorithms/agc.cpp | 2 ++ src/ipa/ipu3/algorithms/agc.h | 3 ++- src/ipa/ipu3/algorithms/awb.cpp | 1 + src/ipa/ipu3/algorithms/awb.h | 3 ++- src/ipa/ipu3/algorithms/tone_mapping.cpp | 2 ++ src/ipa/ipu3/algorithms/tone_mapping.h | 3 ++- src/ipa/ipu3/ipu3.cpp | 2 +- src/ipa/libipa/algorithm.cpp | 1 + src/ipa/libipa/algorithm.h | 1 + src/ipa/rkisp1/algorithms/agc.cpp | 1 + src/ipa/rkisp1/algorithms/agc.h | 3 ++- src/ipa/rkisp1/algorithms/awb.cpp | 1 + src/ipa/rkisp1/algorithms/awb.h | 3 ++- src/ipa/rkisp1/rkisp1.cpp | 2 +- 16 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/ipa/ipu3/algorithms/af.cpp b/src/ipa/ipu3/algorithms/af.cpp index 1faf9296..29a9f520 100644 --- a/src/ipa/ipu3/algorithms/af.cpp +++ b/src/ipa/ipu3/algorithms/af.cpp @@ -409,6 +409,7 @@ bool Af::afIsOutOfFocus(IPAContext context) /** * \brief Determine the max contrast image and lens position. * \param[in] context The IPA context. + * \param[in] frame The frame context sequence number * \param[in] frameContext The current frame context * \param[in] stats The statistics buffer of IPU3. * @@ -424,6 +425,7 @@ bool Af::afIsOutOfFocus(IPAContext context) * [1] Hill Climbing Algorithm, https://en.wikipedia.org/wiki/Hill_climbing */ void Af::process(IPAContext &context, + [[maybe_unused]] unsigned int frame, [[maybe_unused]] IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) { diff --git a/src/ipa/ipu3/algorithms/af.h b/src/ipa/ipu3/algorithms/af.h index 4e2bbd08..97711250 100644 --- a/src/ipa/ipu3/algorithms/af.h +++ b/src/ipa/ipu3/algorithms/af.h @@ -31,10 +31,12 @@ class Af : public Algorithm ~Af() = default; int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; + void prepare(IPAContext &context, unsigned int frame, IPU3FrameContext &frameContext, ipu3_uapi_params *params) override; - void process(IPAContext &context, IPU3FrameContext &frameContext, + void process(IPAContext &context, unsigned int frame, + IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; private: diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 92ea6249..00ef54d4 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -317,6 +317,7 @@ double Agc::estimateLuminance(IPAActiveState &activeState, /** * \brief Process IPU3 statistics, and run AGC operations * \param[in] context The shared IPA context + * \param[in] frame The current frame sequence number * \param[in] frameContext The current frame context * \param[in] stats The IPU3 statistics and ISP results * @@ -324,6 +325,7 @@ double Agc::estimateLuminance(IPAActiveState &activeState, * new exposure and gain for the scene. */ void Agc::process(IPAContext &context, + [[maybe_unused]] unsigned int frame, IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) { diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h index 96585f48..5bb9dc8f 100644 --- a/src/ipa/ipu3/algorithms/agc.h +++ b/src/ipa/ipu3/algorithms/agc.h @@ -28,7 +28,8 @@ class Agc : public Algorithm ~Agc() = default; int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; - void process(IPAContext &context, IPU3FrameContext &frameContext, + void process(IPAContext &context, unsigned int frame, + IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; private: diff --git a/src/ipa/ipu3/algorithms/awb.cpp b/src/ipa/ipu3/algorithms/awb.cpp index b110ad40..8943ead6 100644 --- a/src/ipa/ipu3/algorithms/awb.cpp +++ b/src/ipa/ipu3/algorithms/awb.cpp @@ -388,6 +388,7 @@ void Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats) * \copydoc libcamera::ipa::Algorithm::process */ void Awb::process(IPAContext &context, + [[maybe_unused]] unsigned int frame, [[maybe_unused]] IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) { diff --git a/src/ipa/ipu3/algorithms/awb.h b/src/ipa/ipu3/algorithms/awb.h index 731e5bae..094b1776 100644 --- a/src/ipa/ipu3/algorithms/awb.h +++ b/src/ipa/ipu3/algorithms/awb.h @@ -43,7 +43,8 @@ class Awb : public Algorithm void prepare(IPAContext &context, unsigned int frame, IPU3FrameContext &frameContext, ipu3_uapi_params *params) override; - void process(IPAContext &context, IPU3FrameContext &frameContext, + void process(IPAContext &context, unsigned int frame, + IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; private: diff --git a/src/ipa/ipu3/algorithms/tone_mapping.cpp b/src/ipa/ipu3/algorithms/tone_mapping.cpp index 0951db43..1acbea98 100644 --- a/src/ipa/ipu3/algorithms/tone_mapping.cpp +++ b/src/ipa/ipu3/algorithms/tone_mapping.cpp @@ -76,6 +76,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context, /** * \brief Calculate the tone mapping look up table * \param context The shared IPA context + * \param frame The current frame sequence number * \param frameContext The current frame context * \param stats The IPU3 statistics and ISP results * @@ -83,6 +84,7 @@ void ToneMapping::prepare([[maybe_unused]] IPAContext &context, * our gamma setting. */ void ToneMapping::process(IPAContext &context, + [[maybe_unused]] unsigned int frame, [[maybe_unused]] IPU3FrameContext &frameContext, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) { diff --git a/src/ipa/ipu3/algorithms/tone_mapping.h b/src/ipa/ipu3/algorithms/tone_mapping.h index cfb3de01..a3ab285c 100644 --- a/src/ipa/ipu3/algorithms/tone_mapping.h +++ b/src/ipa/ipu3/algorithms/tone_mapping.h @@ -21,7 +21,8 @@ class ToneMapping : public Algorithm int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; void prepare(IPAContext &context, unsigned int frame, IPU3FrameContext &frameContext, ipu3_uapi_params *params) override; - void process(IPAContext &context, IPU3FrameContext &frameContext, + void process(IPAContext &context, unsigned int frame, + IPU3FrameContext &frameContext, const ipu3_uapi_stats_3a *stats) override; private: diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index ffb9ea8c..da4d416b 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -584,7 +584,7 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, ControlList ctrls(controls::controls); for (auto const &algo : algorithms_) - algo->process(context_, frameContext, stats); + algo->process(context_, frame, frameContext, stats); setControls(frame); diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp index 0992e5de..30eab67f 100644 --- a/src/ipa/libipa/algorithm.cpp +++ b/src/ipa/libipa/algorithm.cpp @@ -103,6 +103,7 @@ namespace ipa { * \fn Algorithm::process() * \brief Process ISP statistics, and run algorithm operations * \param[in] context The shared IPA context + * \param[in] frame The frame context sequence number * \param[in] frameContext The current frame's context * \param[in] stats The IPA statistics and ISP results * diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h index 379207f1..c2d99070 100644 --- a/src/ipa/libipa/algorithm.h +++ b/src/ipa/libipa/algorithm.h @@ -51,6 +51,7 @@ class Algorithm } virtual void process([[maybe_unused]] typename Module::Context &context, + [[maybe_unused]] unsigned int frame, [[maybe_unused]] typename Module::FrameContext &frameContext, [[maybe_unused]] const typename Module::Stats *stats) { diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 2d436511..60018db7 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -281,6 +281,7 @@ double Agc::measureBrightness(const rkisp1_cif_isp_hist_stat *hist) const * new exposure and gain for the scene. */ void Agc::process(IPAContext &context, + [[maybe_unused]] unsigned int frame, [[maybe_unused]] RKISP1FrameContext &frameContext, const rkisp1_stat_buffer *stats) { diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h index 97ca0e31..473dbee5 100644 --- a/src/ipa/rkisp1/algorithms/agc.h +++ b/src/ipa/rkisp1/algorithms/agc.h @@ -29,7 +29,8 @@ class Agc : public Algorithm void prepare(IPAContext &context, unsigned int frame, RKISP1FrameContext &frameContext, rkisp1_params_cfg *params) override; - void process(IPAContext &context, RKISP1FrameContext &frameContext, + void process(IPAContext &context, unsigned int frame, + RKISP1FrameContext &frameContext, const rkisp1_stat_buffer *stats) override; private: diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 1af6d98c..b3ffa6cd 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -123,6 +123,7 @@ void Awb::prepare(IPAContext &context, * \copydoc libcamera::ipa::Algorithm::process */ void Awb::process([[maybe_unused]] IPAContext &context, + [[maybe_unused]] unsigned int frame, [[maybe_unused]] RKISP1FrameContext &frameCtx, const rkisp1_stat_buffer *stats) { diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h index 82d5ce1b..c2b18ea7 100644 --- a/src/ipa/rkisp1/algorithms/awb.h +++ b/src/ipa/rkisp1/algorithms/awb.h @@ -23,7 +23,8 @@ class Awb : public Algorithm void prepare(IPAContext &context, unsigned int frame, RKISP1FrameContext &frameContext, rkisp1_params_cfg *params) override; - void process(IPAContext &context, RKISP1FrameContext &frameCtx, + void process(IPAContext &context, unsigned int frame, + RKISP1FrameContext &frameCtx, const rkisp1_stat_buffer *stats) override; private: diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 091be3fc..22229999 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -338,7 +338,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId unsigned int aeState = 0; for (auto const &algo : algorithms()) - algo->process(context_, frameContext, stats); + algo->process(context_, frame, frameContext, stats); setControls(frame); From 0a30b3cf7cbe42b58b7fdea2842dc6103642a4cd Mon Sep 17 00:00:00 2001 From: Kieran Bingham via libcamera-devel Date: Thu, 21 Jul 2022 13:13:10 +0100 Subject: [PATCH 15/15] ipa: libipa: algorithm: queueRequest(): Pass frame context Pass the current FrameContext for calls to queueRequest(). Now that both the IPU3 and RkISP1 IPA modules comply with the same interface, propagate the queueRequest() call to algorithms in the IPU3 IPA module. Signed-off-by: Kieran Bingham Signed-off-by: Jacopo Mondi --- src/ipa/ipu3/ipu3.cpp | 6 ++---- src/ipa/libipa/algorithm.cpp | 1 + src/ipa/libipa/algorithm.h | 1 + src/ipa/rkisp1/algorithms/cproc.cpp | 1 + src/ipa/rkisp1/algorithms/cproc.h | 1 + src/ipa/rkisp1/algorithms/filter.cpp | 1 + src/ipa/rkisp1/algorithms/filter.h | 1 + src/ipa/rkisp1/rkisp1.cpp | 4 +++- 8 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index da4d416b..2b526ef3 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -619,12 +619,10 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame, */ void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls) { - /* \todo Start processing for 'frame' based on 'controls'. */ IPU3FrameContext &frameContext = context_.frameContexts.initialise(frame); - /* \todo Implement queueRequest to each algorithm. */ - (void)frameContext; - (void)controls; + for (auto const &algo : algorithms_) + algo->queueRequest(context_, frame, frameContext, controls); } /** diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp index 30eab67f..c152b885 100644 --- a/src/ipa/libipa/algorithm.cpp +++ b/src/ipa/libipa/algorithm.cpp @@ -88,6 +88,7 @@ namespace ipa { * \brief Provide control values to the algorithm * \param[in] context The shared IPA context * \param[in] frame The frame number to apply the control values + * \param[in] frameContext The current frame's context * \param[in] controls The list of user controls * * This function is called for each request queued to the camera. It provides diff --git a/src/ipa/libipa/algorithm.h b/src/ipa/libipa/algorithm.h index c2d99070..f92829ad 100644 --- a/src/ipa/libipa/algorithm.h +++ b/src/ipa/libipa/algorithm.h @@ -46,6 +46,7 @@ class Algorithm virtual void queueRequest([[maybe_unused]] typename Module::Context &context, [[maybe_unused]] const uint32_t frame, + [[maybe_unused]] typename Module::FrameContext &frameContext, [[maybe_unused]] const ControlList &controls) { } diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp index edaf2d10..c0792926 100644 --- a/src/ipa/rkisp1/algorithms/cproc.cpp +++ b/src/ipa/rkisp1/algorithms/cproc.cpp @@ -38,6 +38,7 @@ LOG_DEFINE_CATEGORY(RkISP1CProc) */ void ColorProcessing::queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, + [[maybe_unused]] RKISP1FrameContext &frameContext, const ControlList &controls) { auto &cproc = context.activeState.cproc; diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h index fde83f3c..13f8967e 100644 --- a/src/ipa/rkisp1/algorithms/cproc.h +++ b/src/ipa/rkisp1/algorithms/cproc.h @@ -22,6 +22,7 @@ class ColorProcessing : public Algorithm ~ColorProcessing() = default; void queueRequest(IPAContext &context, const uint32_t frame, + RKISP1FrameContext &frameContext, const ControlList &controls) override; void prepare(IPAContext &context, unsigned int frame, RKISP1FrameContext &frameContext, diff --git a/src/ipa/rkisp1/algorithms/filter.cpp b/src/ipa/rkisp1/algorithms/filter.cpp index f6577046..8c5af213 100644 --- a/src/ipa/rkisp1/algorithms/filter.cpp +++ b/src/ipa/rkisp1/algorithms/filter.cpp @@ -44,6 +44,7 @@ static constexpr uint32_t kFiltModeDefault = 0x000004f2; */ void Filter::queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, + [[maybe_unused]] RKISP1FrameContext &frameContext, const ControlList &controls) { auto &filter = context.activeState.filter; diff --git a/src/ipa/rkisp1/algorithms/filter.h b/src/ipa/rkisp1/algorithms/filter.h index 1fbe785a..b10efa20 100644 --- a/src/ipa/rkisp1/algorithms/filter.h +++ b/src/ipa/rkisp1/algorithms/filter.h @@ -22,6 +22,7 @@ class Filter : public Algorithm ~Filter() = default; void queueRequest(IPAContext &context, const uint32_t frame, + RKISP1FrameContext &frameContext, const ControlList &controls) override; void prepare(IPAContext &context, unsigned int frame, RKISP1FrameContext &frameContext, diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 22229999..7821e549 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -299,8 +299,10 @@ void IPARkISP1::unmapBuffers(const std::vector &ids) void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls) { + RKISP1FrameContext &frameContext = context_.frameContexts.initialise(frame); + for (auto const &algo : algorithms()) - algo->queueRequest(context_, frame, controls); + algo->queueRequest(context_, frame, frameContext, controls); } void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)