[libcamera-devel,v4,04/32] ipa: libipa: Introduce FrameContextQueue

17313 diff mbox series

From: Umang Jain <umang.jain@ideasonboard.com>

Introduce a common implementation in libipa to represent the queue of
frame contexts.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
Changes since v3:

- Split the IPU3 changes to a separate patch
- Use composition instead of inheritance
- Use vector instead of array for backend storage
- Make the queue size dynamic
- Rename initialise() to init()
---
 src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++
 src/ipa/libipa/fc_queue.h   | 108 +++++++++++++++++++++++++++++++++
 src/ipa/libipa/meson.build  |   2 +
 3 files changed, 227 insertions(+)
 create mode 100644 src/ipa/libipa/fc_queue.cpp
 create mode 100644 src/ipa/libipa/fc_queue.h

Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:32)
> From: Umang Jain <umang.jain@ideasonboard.com>
> 
> Introduce a common implementation in libipa to represent the queue of
> frame contexts.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> Changes since v3:
> 
> - Split the IPU3 changes to a separate patch
> - Use composition instead of inheritance
> - Use vector instead of array for backend storage
> - Make the queue size dynamic
> - Rename initialise() to init()
> ---
>  src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/fc_queue.h   | 108 +++++++++++++++++++++++++++++++++
>  src/ipa/libipa/meson.build  |   2 +
>  3 files changed, 227 insertions(+)
>  create mode 100644 src/ipa/libipa/fc_queue.cpp
>  create mode 100644 src/ipa/libipa/fc_queue.h
> 
> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> new file mode 100644
> index 000000000000..b81d497e255a
> --- /dev/null
> +++ b/src/ipa/libipa/fc_queue.cpp
> @@ -0,0 +1,117 @@
> +/* 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 <libcamera/base/log.h>
> +
> +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 FrameContext instances through the IPA
> + * \tparam FrameContext The IPA specific FrameContext derived class type

; or , after FrameContext?

> + *
> + * The frame context queue provides a simple circular buffer implementation to
> + * store IPA specific context for each frame through its lifetime within the
> + * IPA.
> + *
> + * 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
> + * 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 frame context implementations shall inherit from the
> + * IPAFrameContext base class to support the minimum required features for a
> + * FrameContext.
> + *
> + * FrameContexts are overwritten when they are recycled and re-initialised by
> + * the first access made on them by either init(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 no way the controls associated with the late Request could be
> + * applied in time, as the frame as already been processed by the IPA.
> + *
> + * \todo Set an error flag for per-frame control errors.
> + */
> +
> +/**
> + * \fn FCQueue::FCQueue(unsigned int size)
> + * \brief Construct a frame context queue of a specified size
> + * \param[in] size The number of contexts in the queue
> + */
> +
> +/**
> + * \fn FCQueue::clear()
> + * \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().
> + */
> +
> +/**
> + * \fn FCQueue::init(uint32_t frame)

I saw you've questioned the name here.

I don't mind if this is init(), alloc() new() ?

We don't (yet?) have an explicit free/delete/release of the FCQueue, as
they stay in memory until something overwrites them like an LRU....


> + * \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.
> + *
> + * 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
> + */
> +
> +/**
> + * \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 initialised for the \a frame, it will be
> + * initialised.
> + *
> + * \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 000000000000..0f3af0f3a189
> --- /dev/null
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * fc_queue.h - IPA Frame context queue
> + */
> +
> +#pragma once
> +
> +#include <algorithm>
> +#include <vector>
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(FCQueue)
> +
> +namespace ipa {
> +
> +template<typename FrameContext>
> +class FCQueue
> +{
> +public:
> +       FCQueue(unsigned int size)
> +               : contexts_(size)
> +       {
> +       }
> +
> +       void clear()
> +       {
> +               std::fill(contexts_.begin(), contexts_.end(), FrameContext{});
> +       }
> +
> +       FrameContext &init(const uint32_t frame)
> +       {
> +               FrameContext &frameContext = contexts_[frame % contexts_.size()];
> +
> +               /*
> +                * Do not re-initialise if a get() call has already fetched this
> +                * frame context to preseve the context.
> +                *
> +                * \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)
> +                               << "Frame " << frame << " already initialised";
> +               else
> +                       init(frameContext, frame);
> +
> +               return frameContext;
> +       }
> +
> +       FrameContext &get(uint32_t frame)
> +       {
> +               FrameContext &frameContext = contexts_[frame % contexts_.size()];
> +
> +               /*
> +                * 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 Context for " << frame
> +                                           << " has been overwritten by "
> +                                           << frameContext.frame;
> +
> +               if (frame == frameContext.frame)
> +                       return frameContext;
> +
> +               /*
> +                * 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

double space between 'IPA.  Controls'

> +                * left unhandled.
> +                *
> +                * \todo Set an error flag for per-frame control errors.
> +                */
> +               LOG(FCQueue, Warning)
> +                       << "Obtained an uninitialised FrameContext for " << frame;
> +
> +               init(frameContext, frame);
> +
> +               return frameContext;
> +       }
> +
> +private:
> +       void init(FrameContext &frameContext, const uint32_t frame)
> +       {
> +               frameContext = {};
> +               frameContext.frame = frame;

Aha this is better.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +       }
> +
> +       std::vector<FrameContext> contexts_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index fb894bc614af..016b8e0ec9be 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',
>  ])
> -- 
> Regards,
> 
> Laurent Pinchart
>

Hi Kieran,

On Tue, Sep 20, 2022 at 02:38:05PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:32)
> > From: Umang Jain <umang.jain@ideasonboard.com>
> > 
> > Introduce a common implementation in libipa to represent the queue of
> > frame contexts.
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > Changes since v3:
> > 
> > - Split the IPU3 changes to a separate patch
> > - Use composition instead of inheritance
> > - Use vector instead of array for backend storage
> > - Make the queue size dynamic
> > - Rename initialise() to init()
> > ---
> >  src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++
> >  src/ipa/libipa/fc_queue.h   | 108 +++++++++++++++++++++++++++++++++
> >  src/ipa/libipa/meson.build  |   2 +
> >  3 files changed, 227 insertions(+)
> >  create mode 100644 src/ipa/libipa/fc_queue.cpp
> >  create mode 100644 src/ipa/libipa/fc_queue.h
> > 
> > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > new file mode 100644
> > index 000000000000..b81d497e255a
> > --- /dev/null
> > +++ b/src/ipa/libipa/fc_queue.cpp
> > @@ -0,0 +1,117 @@
> > +/* 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 <libcamera/base/log.h>
> > +
> > +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 FrameContext instances through the IPA
> > + * \tparam FrameContext The IPA specific FrameContext derived class type

I'll s/IPA specific/IPA-specific/

> ; or , after FrameContext?

Which one, the first or second ? The first FrameContext is the template
parameter name and thus shouldn't be followed by anything, and the
second one doesn't seem to require a comma.

> > + *
> > + * The frame context queue provides a simple circular buffer implementation to
> > + * store IPA specific context for each frame through its lifetime within the
> > + * IPA.
> > + *
> > + * 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
> > + * 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 frame context implementations shall inherit from the
> > + * IPAFrameContext base class to support the minimum required features for a
> > + * FrameContext.
> > + *
> > + * FrameContexts are overwritten when they are recycled and re-initialised by
> > + * the first access made on them by either init(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 no way the controls associated with the late Request could be
> > + * applied in time, as the frame as already been processed by the IPA.
> > + *
> > + * \todo Set an error flag for per-frame control errors.
> > + */
> > +
> > +/**
> > + * \fn FCQueue::FCQueue(unsigned int size)
> > + * \brief Construct a frame context queue of a specified size
> > + * \param[in] size The number of contexts in the queue
> > + */
> > +
> > +/**
> > + * \fn FCQueue::clear()
> > + * \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().
> > + */
> > +
> > +/**
> > + * \fn FCQueue::init(uint32_t frame)
> 
> I saw you've questioned the name here.
> 
> I don't mind if this is init(), alloc() new() ?
> 
> We don't (yet?) have an explicit free/delete/release of the FCQueue, as
> they stay in memory until something overwrites them like an LRU....

We shouldn't allocate memory dynamically, so there will be no memory
allocation as such, but the concept of allocation a frame context from
the queue still applies. I indeed prefer alloc() over init(). Better
names are also possible. I'll see what I can do in the next version.

> > + * \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.
> > + *
> > + * 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
> > + */
> > +
> > +/**
> > + * \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 initialised for the \a frame, it will be
> > + * initialised.
> > + *
> > + * \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 000000000000..0f3af0f3a189
> > --- /dev/null
> > +++ b/src/ipa/libipa/fc_queue.h
> > @@ -0,0 +1,108 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * fc_queue.h - IPA Frame context queue
> > + */
> > +
> > +#pragma once
> > +
> > +#include <algorithm>
> > +#include <vector>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(FCQueue)
> > +
> > +namespace ipa {
> > +
> > +template<typename FrameContext>
> > +class FCQueue
> > +{
> > +public:
> > +       FCQueue(unsigned int size)
> > +               : contexts_(size)
> > +       {
> > +       }
> > +
> > +       void clear()
> > +       {
> > +               std::fill(contexts_.begin(), contexts_.end(), FrameContext{});
> > +       }
> > +
> > +       FrameContext &init(const uint32_t frame)
> > +       {
> > +               FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > +
> > +               /*
> > +                * Do not re-initialise if a get() call has already fetched this
> > +                * frame context to preseve the context.
> > +                *
> > +                * \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)
> > +                               << "Frame " << frame << " already initialised";
> > +               else
> > +                       init(frameContext, frame);
> > +
> > +               return frameContext;
> > +       }
> > +
> > +       FrameContext &get(uint32_t frame)
> > +       {
> > +               FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > +
> > +               /*
> > +                * 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 Context for " << frame
> > +                                           << " has been overwritten by "
> > +                                           << frameContext.frame;
> > +
> > +               if (frame == frameContext.frame)
> > +                       return frameContext;
> > +
> > +               /*
> > +                * 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
> 
> double space between 'IPA.  Controls'
> 
> > +                * left unhandled.
> > +                *
> > +                * \todo Set an error flag for per-frame control errors.
> > +                */
> > +               LOG(FCQueue, Warning)
> > +                       << "Obtained an uninitialised FrameContext for " << frame;
> > +
> > +               init(frameContext, frame);
> > +
> > +               return frameContext;
> > +       }
> > +
> > +private:
> > +       void init(FrameContext &frameContext, const uint32_t frame)
> > +       {
> > +               frameContext = {};
> > +               frameContext.frame = frame;
> 
> Aha this is better.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +       }
> > +
> > +       std::vector<FrameContext> contexts_;
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index fb894bc614af..016b8e0ec9be 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',
> >  ])

Quoting Laurent Pinchart (2022-09-20 20:11:10)
> Hi Kieran,
> 
> On Tue, Sep 20, 2022 at 02:38:05PM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:32)
> > > From: Umang Jain <umang.jain@ideasonboard.com>
> > > 
> > > Introduce a common implementation in libipa to represent the queue of
> > > frame contexts.
> > > 
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > Changes since v3:
> > > 
> > > - Split the IPU3 changes to a separate patch
> > > - Use composition instead of inheritance
> > > - Use vector instead of array for backend storage
> > > - Make the queue size dynamic
> > > - Rename initialise() to init()
> > > ---
> > >  src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++
> > >  src/ipa/libipa/fc_queue.h   | 108 +++++++++++++++++++++++++++++++++
> > >  src/ipa/libipa/meson.build  |   2 +
> > >  3 files changed, 227 insertions(+)
> > >  create mode 100644 src/ipa/libipa/fc_queue.cpp
> > >  create mode 100644 src/ipa/libipa/fc_queue.h
> > > 
> > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > > new file mode 100644
> > > index 000000000000..b81d497e255a
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/fc_queue.cpp
> > > @@ -0,0 +1,117 @@
> > > +/* 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 <libcamera/base/log.h>
> > > +
> > > +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 FrameContext instances through the IPA
> > > + * \tparam FrameContext The IPA specific FrameContext derived class type
> 
> I'll s/IPA specific/IPA-specific/

Sure

> 
> > ; or , after FrameContext?
> 
> Which one, the first or second ? The first FrameContext is the template
> parameter name and thus shouldn't be followed by anything, and the
> second one doesn't seem to require a comma.

The first. It reads like:

A vehicle for transporting Maize samples through the processing factory
\tparam Factory The place where maize is processed.

You've gone from a type to a capital "The" which is describing the
previous item, without a pause.

I have no idea how to 'name' that - but it triggers the big red alarm
for "This doesn't look right in native English".

> > > + * The frame context queue provides a simple circular buffer implementation to
> > > + * store IPA specific context for each frame through its lifetime within the
> > > + * IPA.
> > > + *
> > > + * 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
> > > + * 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 frame context implementations shall inherit from the
> > > + * IPAFrameContext base class to support the minimum required features for a
> > > + * FrameContext.
> > > + *
> > > + * FrameContexts are overwritten when they are recycled and re-initialised by
> > > + * the first access made on them by either init(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 no way the controls associated with the late Request could be
> > > + * applied in time, as the frame as already been processed by the IPA.
> > > + *
> > > + * \todo Set an error flag for per-frame control errors.
> > > + */
> > > +
> > > +/**
> > > + * \fn FCQueue::FCQueue(unsigned int size)
> > > + * \brief Construct a frame context queue of a specified size
> > > + * \param[in] size The number of contexts in the queue
> > > + */
> > > +
> > > +/**
> > > + * \fn FCQueue::clear()
> > > + * \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().
> > > + */
> > > +
> > > +/**
> > > + * \fn FCQueue::init(uint32_t frame)
> > 
> > I saw you've questioned the name here.
> > 
> > I don't mind if this is init(), alloc() new() ?
> > 
> > We don't (yet?) have an explicit free/delete/release of the FCQueue, as
> > they stay in memory until something overwrites them like an LRU....
> 
> We shouldn't allocate memory dynamically, so there will be no memory
> allocation as such, but the concept of allocation a frame context from
> the queue still applies. I indeed prefer alloc() over init(). Better
> names are also possible. I'll see what I can do in the next version.
> 
> > > + * \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.
> > > + *
> > > + * 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
> > > + */
> > > +
> > > +/**
> > > + * \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 initialised for the \a frame, it will be
> > > + * initialised.
> > > + *
> > > + * \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 000000000000..0f3af0f3a189
> > > --- /dev/null
> > > +++ b/src/ipa/libipa/fc_queue.h
> > > @@ -0,0 +1,108 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2022, Google Inc.
> > > + *
> > > + * fc_queue.h - IPA Frame context queue
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <algorithm>
> > > +#include <vector>
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DECLARE_CATEGORY(FCQueue)
> > > +
> > > +namespace ipa {
> > > +
> > > +template<typename FrameContext>
> > > +class FCQueue
> > > +{
> > > +public:
> > > +       FCQueue(unsigned int size)
> > > +               : contexts_(size)
> > > +       {
> > > +       }
> > > +
> > > +       void clear()
> > > +       {
> > > +               std::fill(contexts_.begin(), contexts_.end(), FrameContext{});
> > > +       }
> > > +
> > > +       FrameContext &init(const uint32_t frame)
> > > +       {
> > > +               FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > > +
> > > +               /*
> > > +                * Do not re-initialise if a get() call has already fetched this
> > > +                * frame context to preseve the context.
> > > +                *
> > > +                * \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)
> > > +                               << "Frame " << frame << " already initialised";
> > > +               else
> > > +                       init(frameContext, frame);
> > > +
> > > +               return frameContext;
> > > +       }
> > > +
> > > +       FrameContext &get(uint32_t frame)
> > > +       {
> > > +               FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > > +
> > > +               /*
> > > +                * 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 Context for " << frame
> > > +                                           << " has been overwritten by "
> > > +                                           << frameContext.frame;
> > > +
> > > +               if (frame == frameContext.frame)
> > > +                       return frameContext;
> > > +
> > > +               /*
> > > +                * 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
> > 
> > double space between 'IPA.  Controls'
> > 
> > > +                * left unhandled.
> > > +                *
> > > +                * \todo Set an error flag for per-frame control errors.
> > > +                */
> > > +               LOG(FCQueue, Warning)
> > > +                       << "Obtained an uninitialised FrameContext for " << frame;
> > > +
> > > +               init(frameContext, frame);
> > > +
> > > +               return frameContext;
> > > +       }
> > > +
> > > +private:
> > > +       void init(FrameContext &frameContext, const uint32_t frame)
> > > +       {
> > > +               frameContext = {};
> > > +               frameContext.frame = frame;
> > 
> > Aha this is better.
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > > +       }
> > > +
> > > +       std::vector<FrameContext> contexts_;
> > > +};
> > > +
> > > +} /* namespace ipa */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > index fb894bc614af..016b8e0ec9be 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',
> > >  ])
> 
> -- 
> Regards,
> 
> Laurent Pinchart

On Tue, Sep 20, 2022 at 11:25:37PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-09-20 20:11:10)
> > On Tue, Sep 20, 2022 at 02:38:05PM +0100, Kieran Bingham wrote:
> > > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:32)
> > > > From: Umang Jain <umang.jain@ideasonboard.com>
> > > > 
> > > > Introduce a common implementation in libipa to represent the queue of
> > > > frame contexts.
> > > > 
> > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > > Changes since v3:
> > > > 
> > > > - Split the IPU3 changes to a separate patch
> > > > - Use composition instead of inheritance
> > > > - Use vector instead of array for backend storage
> > > > - Make the queue size dynamic
> > > > - Rename initialise() to init()
> > > > ---
> > > >  src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++
> > > >  src/ipa/libipa/fc_queue.h   | 108 +++++++++++++++++++++++++++++++++
> > > >  src/ipa/libipa/meson.build  |   2 +
> > > >  3 files changed, 227 insertions(+)
> > > >  create mode 100644 src/ipa/libipa/fc_queue.cpp
> > > >  create mode 100644 src/ipa/libipa/fc_queue.h
> > > > 
> > > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > > > new file mode 100644
> > > > index 000000000000..b81d497e255a
> > > > --- /dev/null
> > > > +++ b/src/ipa/libipa/fc_queue.cpp
> > > > @@ -0,0 +1,117 @@
> > > > +/* 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 <libcamera/base/log.h>
> > > > +
> > > > +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 FrameContext instances through the IPA
> > > > + * \tparam FrameContext The IPA specific FrameContext derived class type
> > 
> > I'll s/IPA specific/IPA-specific/
> 
> Sure
> 
> > > ; or , after FrameContext?
> > 
> > Which one, the first or second ? The first FrameContext is the template
> > parameter name and thus shouldn't be followed by anything, and the
> > second one doesn't seem to require a comma.
> 
> The first. It reads like:
> 
> A vehicle for transporting Maize samples through the processing factory
> \tparam Factory The place where maize is processed.
> 
> You've gone from a type to a capital "The" which is describing the
> previous item, without a pause.
> 
> I have no idea how to 'name' that - but it triggers the big red alarm
> for "This doesn't look right in native English".

\tparam is like \param, but for template parameters, not function
parameters. The \brief and \tparam lines are distinct, the second isn't
a continuation of the first.

> > > > + * The frame context queue provides a simple circular buffer implementation to
> > > > + * store IPA specific context for each frame through its lifetime within the
> > > > + * IPA.
> > > > + *
> > > > + * 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
> > > > + * 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 frame context implementations shall inherit from the
> > > > + * IPAFrameContext base class to support the minimum required features for a
> > > > + * FrameContext.
> > > > + *
> > > > + * FrameContexts are overwritten when they are recycled and re-initialised by
> > > > + * the first access made on them by either init(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 no way the controls associated with the late Request could be
> > > > + * applied in time, as the frame as already been processed by the IPA.
> > > > + *
> > > > + * \todo Set an error flag for per-frame control errors.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn FCQueue::FCQueue(unsigned int size)
> > > > + * \brief Construct a frame context queue of a specified size
> > > > + * \param[in] size The number of contexts in the queue
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn FCQueue::clear()
> > > > + * \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().
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn FCQueue::init(uint32_t frame)
> > > 
> > > I saw you've questioned the name here.
> > > 
> > > I don't mind if this is init(), alloc() new() ?
> > > 
> > > We don't (yet?) have an explicit free/delete/release of the FCQueue, as
> > > they stay in memory until something overwrites them like an LRU....
> > 
> > We shouldn't allocate memory dynamically, so there will be no memory
> > allocation as such, but the concept of allocation a frame context from
> > the queue still applies. I indeed prefer alloc() over init(). Better
> > names are also possible. I'll see what I can do in the next version.
> > 
> > > > + * \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.
> > > > + *
> > > > + * 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
> > > > + */
> > > > +
> > > > +/**
> > > > + * \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 initialised for the \a frame, it will be
> > > > + * initialised.
> > > > + *
> > > > + * \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 000000000000..0f3af0f3a189
> > > > --- /dev/null
> > > > +++ b/src/ipa/libipa/fc_queue.h
> > > > @@ -0,0 +1,108 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2022, Google Inc.
> > > > + *
> > > > + * fc_queue.h - IPA Frame context queue
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include <algorithm>
> > > > +#include <vector>
> > > > +
> > > > +#include <libcamera/base/log.h>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +LOG_DECLARE_CATEGORY(FCQueue)
> > > > +
> > > > +namespace ipa {
> > > > +
> > > > +template<typename FrameContext>
> > > > +class FCQueue
> > > > +{
> > > > +public:
> > > > +       FCQueue(unsigned int size)
> > > > +               : contexts_(size)
> > > > +       {
> > > > +       }
> > > > +
> > > > +       void clear()
> > > > +       {
> > > > +               std::fill(contexts_.begin(), contexts_.end(), FrameContext{});
> > > > +       }
> > > > +
> > > > +       FrameContext &init(const uint32_t frame)
> > > > +       {
> > > > +               FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > > > +
> > > > +               /*
> > > > +                * Do not re-initialise if a get() call has already fetched this
> > > > +                * frame context to preseve the context.
> > > > +                *
> > > > +                * \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)
> > > > +                               << "Frame " << frame << " already initialised";
> > > > +               else
> > > > +                       init(frameContext, frame);
> > > > +
> > > > +               return frameContext;
> > > > +       }
> > > > +
> > > > +       FrameContext &get(uint32_t frame)
> > > > +       {
> > > > +               FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > > > +
> > > > +               /*
> > > > +                * 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 Context for " << frame
> > > > +                                           << " has been overwritten by "
> > > > +                                           << frameContext.frame;
> > > > +
> > > > +               if (frame == frameContext.frame)
> > > > +                       return frameContext;
> > > > +
> > > > +               /*
> > > > +                * 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
> > > 
> > > double space between 'IPA.  Controls'
> > > 
> > > > +                * left unhandled.
> > > > +                *
> > > > +                * \todo Set an error flag for per-frame control errors.
> > > > +                */
> > > > +               LOG(FCQueue, Warning)
> > > > +                       << "Obtained an uninitialised FrameContext for " << frame;
> > > > +
> > > > +               init(frameContext, frame);
> > > > +
> > > > +               return frameContext;
> > > > +       }
> > > > +
> > > > +private:
> > > > +       void init(FrameContext &frameContext, const uint32_t frame)
> > > > +       {
> > > > +               frameContext = {};
> > > > +               frameContext.frame = frame;
> > > 
> > > Aha this is better.
> > > 
> > > 
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > > +       }
> > > > +
> > > > +       std::vector<FrameContext> contexts_;
> > > > +};
> > > > +
> > > > +} /* namespace ipa */
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > > > index fb894bc614af..016b8e0ec9be 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',
> > > >  ])

Quoting Laurent Pinchart (2022-09-20 23:29:56)
> On Tue, Sep 20, 2022 at 11:25:37PM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2022-09-20 20:11:10)
> > > On Tue, Sep 20, 2022 at 02:38:05PM +0100, Kieran Bingham wrote:
> > > > Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:32)
> > > > > From: Umang Jain <umang.jain@ideasonboard.com>
> > > > > 
> > > > > Introduce a common implementation in libipa to represent the queue of
> > > > > frame contexts.
> > > > > 
> > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > ---
> > > > > Changes since v3:
> > > > > 
> > > > > - Split the IPU3 changes to a separate patch
> > > > > - Use composition instead of inheritance
> > > > > - Use vector instead of array for backend storage
> > > > > - Make the queue size dynamic
> > > > > - Rename initialise() to init()
> > > > > ---
> > > > >  src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++
> > > > >  src/ipa/libipa/fc_queue.h   | 108 +++++++++++++++++++++++++++++++++
> > > > >  src/ipa/libipa/meson.build  |   2 +
> > > > >  3 files changed, 227 insertions(+)
> > > > >  create mode 100644 src/ipa/libipa/fc_queue.cpp
> > > > >  create mode 100644 src/ipa/libipa/fc_queue.h
> > > > > 
> > > > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > > > > new file mode 100644
> > > > > index 000000000000..b81d497e255a
> > > > > --- /dev/null
> > > > > +++ b/src/ipa/libipa/fc_queue.cpp
> > > > > @@ -0,0 +1,117 @@
> > > > > +/* 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 <libcamera/base/log.h>
> > > > > +
> > > > > +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 FrameContext instances through the IPA
> > > > > + * \tparam FrameContext The IPA specific FrameContext derived class type
> > > 
> > > I'll s/IPA specific/IPA-specific/
> > 
> > Sure
> > 
> > > > ; or , after FrameContext?
> > > 
> > > Which one, the first or second ? The first FrameContext is the template
> > > parameter name and thus shouldn't be followed by anything, and the
> > > second one doesn't seem to require a comma.
> > 
> > The first. It reads like:
> > 
> > A vehicle for transporting Maize samples through the processing factory
> > \tparam Factory The place where maize is processed.
> > 
> > You've gone from a type to a capital "The" which is describing the
> > previous item, without a pause.
> > 
> > I have no idea how to 'name' that - but it triggers the big red alarm
> > for "This doesn't look right in native English".
> 
> \tparam is like \param, but for template parameters, not function
> parameters. The \brief and \tparam lines are distinct, the second isn't
> a continuation of the first.

Argh - yes - I was reading the line like a continuation.

I'll go back to sleep ;-) But now I'll put up more of a fight for
indentation on continued lines ...

--
Kieran

Hi Laurent

On Thu, Sep 08, 2022 at 04:41:32AM +0300, Laurent Pinchart via libcamera-devel wrote:
> From: Umang Jain <umang.jain@ideasonboard.com>
>
> Introduce a common implementation in libipa to represent the queue of
> frame contexts.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> Changes since v3:
>
> - Split the IPU3 changes to a separate patch
> - Use composition instead of inheritance



> - Use vector instead of array for backend storage

I guess this is because now the FCQ size has dynamic size ?

> - Make the queue size dynamic
> - Rename initialise() to init()
> ---
>  src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++
>  src/ipa/libipa/fc_queue.h   | 108 +++++++++++++++++++++++++++++++++
>  src/ipa/libipa/meson.build  |   2 +
>  3 files changed, 227 insertions(+)
>  create mode 100644 src/ipa/libipa/fc_queue.cpp
>  create mode 100644 src/ipa/libipa/fc_queue.h
>
> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> new file mode 100644
> index 000000000000..b81d497e255a
> --- /dev/null
> +++ b/src/ipa/libipa/fc_queue.cpp
> @@ -0,0 +1,117 @@
> +/* 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 <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(FCQueue)
> +
> +namespace ipa {
> +
> +/**
> + * \file fc_queue.h
> + * \brief Queue to access per-frame Context

contexts maybe, or does Context name a type

> + */
> +
> +/**
> + * \class FCQueue
> + * \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
> + * store IPA specific context for each frame through its lifetime within the
> + * IPA.
> + *
> + * 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
> + * 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

"A frame sequence number" or "The frame sequence number" ?

> + * 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 frame context implementations shall inherit from the
> + * IPAFrameContext base class to support the minimum required features for a
> + * FrameContext.
> + *
> + * FrameContexts are overwritten when they are recycled and re-initialised by
> + * the first access made on them by either init(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 no way the controls associated with the late Request could be
> + * applied in time, as the frame as already been processed by the IPA.

s/as already/has already/

> + *
> + * \todo Set an error flag for per-frame control errors.
> + */
> +
> +/**
> + * \fn FCQueue::FCQueue(unsigned int size)
> + * \brief Construct a frame context queue of a specified size
> + * \param[in] size The number of contexts in the queue
> + */
> +
> +/**
> + * \fn FCQueue::clear()
> + * \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().

I recall we had a discussion and if I'm not mistaken clearing the FCQ at
start() time would create issues with Requests queued to the camera
before Camera::start() was called.

iirc we settled on stop() as the "right" place to clear the FCQ
between streaming sessions (assuming it gets initialized in a clean
state ofc).

However the doc here matches the v3 version.

> + */
> +
> +/**
> + * \fn FCQueue::init(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.

This sounds like "it is returned to the caller if it was not already
initialised."

What about "The FrameContext will be initialised, if not initialised
already, and returned to the caller."

> + *
> + * 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
> + */
> +
> +/**
> + * \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 initialised for the \a frame, it will be
> + * initialised.
> + *
> + * \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 000000000000..0f3af0f3a189
> --- /dev/null
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2022, Google Inc.
> + *
> + * fc_queue.h - IPA Frame context queue
> + */
> +
> +#pragma once
> +
> +#include <algorithm>
> +#include <vector>
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(FCQueue)
> +
> +namespace ipa {
> +
> +template<typename FrameContext>
> +class FCQueue
> +{
> +public:
> +	FCQueue(unsigned int size)
> +		: contexts_(size)

I'm not sure I get why the FCQ size should be dynamic. If I recall
correctly Umang had this on v2 ? The size of the FCQ is a camera
property which corresponds to the max number of requests in flight
a camera can handle and its fixed at compile time ?

Using a libcamera::property also guarantee that the two sizes matches.
Is it too demanding for IPA implementations ?

> +	{
> +	}
> +
> +	void clear()
> +	{
> +		std::fill(contexts_.begin(), contexts_.end(), FrameContext{});
> +	}
> +
> +	FrameContext &init(const uint32_t frame)
> +	{
> +		FrameContext &frameContext = contexts_[frame % contexts_.size()];
> +
> +		/*
> +		 * Do not re-initialise if a get() call has already fetched this
> +		 * frame context to preseve the context.
> +		 *
> +		 * \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)
> +				<< "Frame " << frame << " already initialised";
> +		else
> +			init(frameContext, frame);
> +
> +		return frameContext;
> +	}
> +
> +	FrameContext &get(uint32_t frame)
> +	{
> +		FrameContext &frameContext = contexts_[frame % contexts_.size()];
> +
> +		/*
> +		 * 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 Context for " << frame
> +					    << " has been overwritten by "
> +					    << frameContext.frame;
> +
> +		if (frame == frameContext.frame)
> +			return frameContext;
> +
> +		/*
> +		 * 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.
> +		 *
> +		 * \todo Set an error flag for per-frame control errors.
> +		 */
> +		LOG(FCQueue, Warning)
> +			<< "Obtained an uninitialised FrameContext for " << frame;
> +
> +		init(frameContext, frame);
> +
> +		return frameContext;
> +	}
> +
> +private:
> +	void init(FrameContext &frameContext, const uint32_t frame)
> +	{
> +		frameContext = {};
> +		frameContext.frame = frame;
> +	}
> +
> +	std::vector<FrameContext> contexts_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index fb894bc614af..016b8e0ec9be 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',
>  ])
> --
> Regards,
>
> Laurent Pinchart
>

Hi Jacopo,

On Wed, Sep 21, 2022 at 07:49:12PM +0200, Jacopo Mondi wrote:
> On Thu, Sep 08, 2022 at 04:41:32AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > From: Umang Jain <umang.jain@ideasonboard.com>
> >
> > Introduce a common implementation in libipa to represent the queue of
> > frame contexts.
> >
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > Changes since v3:
> >
> > - Split the IPU3 changes to a separate patch
> > - Use composition instead of inheritance
> 
> 
> 
> > - Use vector instead of array for backend storage
> 
> I guess this is because now the FCQ size has dynamic size ?

That's right.

> > - Make the queue size dynamic
> > - Rename initialise() to init()
> > ---
> >  src/ipa/libipa/fc_queue.cpp | 117 ++++++++++++++++++++++++++++++++++++
> >  src/ipa/libipa/fc_queue.h   | 108 +++++++++++++++++++++++++++++++++
> >  src/ipa/libipa/meson.build  |   2 +
> >  3 files changed, 227 insertions(+)
> >  create mode 100644 src/ipa/libipa/fc_queue.cpp
> >  create mode 100644 src/ipa/libipa/fc_queue.h
> >
> > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > new file mode 100644
> > index 000000000000..b81d497e255a
> > --- /dev/null
> > +++ b/src/ipa/libipa/fc_queue.cpp
> > @@ -0,0 +1,117 @@
> > +/* 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 <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(FCQueue)
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \file fc_queue.h
> > + * \brief Queue to access per-frame Context
> 
> contexts maybe, or does Context name a type

Maybe Context was a type at some point, or maybe I considered turning it
into a type, but it's not now, so I'll use "contexts". I'll also replace
"Frame Context" with "FrameContext" below.

> > + */
> > +
> > +/**
> > + * \class FCQueue
> > + * \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
> > + * store IPA specific context for each frame through its lifetime within the
> > + * IPA.
> > + *
> > + * 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
> > + * 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
> 
> "A frame sequence number" or "The frame sequence number" ?

Ack.

> > + * 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 frame context implementations shall inherit from the
> > + * IPAFrameContext base class to support the minimum required features for a
> > + * FrameContext.
> > + *
> > + * FrameContexts are overwritten when they are recycled and re-initialised by
> > + * the first access made on them by either init(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 no way the controls associated with the late Request could be
> > + * applied in time, as the frame as already been processed by the IPA.
> 
> s/as already/has already/
> 
> > + *
> > + * \todo Set an error flag for per-frame control errors.
> > + */
> > +
> > +/**
> > + * \fn FCQueue::FCQueue(unsigned int size)
> > + * \brief Construct a frame context queue of a specified size
> > + * \param[in] size The number of contexts in the queue
> > + */
> > +
> > +/**
> > + * \fn FCQueue::clear()
> > + * \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().
> 
> I recall we had a discussion and if I'm not mistaken clearing the FCQ at
> start() time would create issues with Requests queued to the camera
> before Camera::start() was called.
> 
> iirc we settled on stop() as the "right" place to clear the FCQ
> between streaming sessions (assuming it gets initialized in a clean
> state ofc).
> 
> However the doc here matches the v3 version.

That's a good point. I'll see if I can fix it.

> > + */
> > +
> > +/**
> > + * \fn FCQueue::init(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.
> 
> This sounds like "it is returned to the caller if it was not already
> initialised."
> 
> What about "The FrameContext will be initialised, if not initialised
> already, and returned to the caller."

Ack.

> > + *
> > + * 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
> > + */
> > +
> > +/**
> > + * \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 initialised for the \a frame, it will be
> > + * initialised.
> > + *
> > + * \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 000000000000..0f3af0f3a189
> > --- /dev/null
> > +++ b/src/ipa/libipa/fc_queue.h
> > @@ -0,0 +1,108 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2022, Google Inc.
> > + *
> > + * fc_queue.h - IPA Frame context queue
> > + */
> > +
> > +#pragma once
> > +
> > +#include <algorithm>
> > +#include <vector>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(FCQueue)
> > +
> > +namespace ipa {
> > +
> > +template<typename FrameContext>
> > +class FCQueue
> > +{
> > +public:
> > +	FCQueue(unsigned int size)
> > +		: contexts_(size)
> 
> I'm not sure I get why the FCQ size should be dynamic. If I recall
> correctly Umang had this on v2 ? The size of the FCQ is a camera
> property which corresponds to the max number of requests in flight
> a camera can handle and its fixed at compile time ?
> 
> Using a libcamera::property also guarantee that the two sizes matches.
> Is it too demanding for IPA implementations ?

Good point. I wanted to allow different IPA modules to use different
context queue sizes, and didn't consider making this a template
parameter. I'll check what it entails.

> > +	{
> > +	}
> > +
> > +	void clear()
> > +	{
> > +		std::fill(contexts_.begin(), contexts_.end(), FrameContext{});
> > +	}
> > +
> > +	FrameContext &init(const uint32_t frame)
> > +	{
> > +		FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > +
> > +		/*
> > +		 * Do not re-initialise if a get() call has already fetched this
> > +		 * frame context to preseve the context.
> > +		 *
> > +		 * \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)
> > +				<< "Frame " << frame << " already initialised";
> > +		else
> > +			init(frameContext, frame);
> > +
> > +		return frameContext;
> > +	}
> > +
> > +	FrameContext &get(uint32_t frame)
> > +	{
> > +		FrameContext &frameContext = contexts_[frame % contexts_.size()];
> > +
> > +		/*
> > +		 * 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 Context for " << frame
> > +					    << " has been overwritten by "
> > +					    << frameContext.frame;
> > +
> > +		if (frame == frameContext.frame)
> > +			return frameContext;
> > +
> > +		/*
> > +		 * 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.
> > +		 *
> > +		 * \todo Set an error flag for per-frame control errors.
> > +		 */
> > +		LOG(FCQueue, Warning)
> > +			<< "Obtained an uninitialised FrameContext for " << frame;
> > +
> > +		init(frameContext, frame);
> > +
> > +		return frameContext;
> > +	}
> > +
> > +private:
> > +	void init(FrameContext &frameContext, const uint32_t frame)
> > +	{
> > +		frameContext = {};
> > +		frameContext.frame = frame;
> > +	}
> > +
> > +	std::vector<FrameContext> contexts_;
> > +};
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index fb894bc614af..016b8e0ec9be 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',
> >  ])