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

[Kieran Bingham]

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 at ideasonboard.com>
> > > > > 
> > > > > Introduce a common implementation in libipa to represent the queue of
> > > > > frame contexts.
> > > > > 
> > > > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > > Signed-off-by: Jacopo Mondi <jacopo at 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