[libcamera-devel,v4,05/32] ipa: libipa: Provide a common base for frame contexts

17314 diff mbox series

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide a common FrameContext as a base for IPA modules to inherit from.

This will allow having a common set of parameters for every frame
context managed by the FCQueue implementation.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
Changes since v3:

- Rename structure from IPAFrameContext to FrameContext
- Move changes to IPA modules to separate patches
- Make the FrameContext::frame member private
---
 src/ipa/libipa/fc_queue.cpp | 18 ++++++++++++++++++
 src/ipa/libipa/fc_queue.h   | 10 ++++++++++
 2 files changed, 28 insertions(+)

Quoting Laurent Pinchart (2022-09-08 02:41:33)
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Provide a common FrameContext as a base for IPA modules to inherit from.
> 
> This will allow having a common set of parameters for every frame
> context managed by the FCQueue implementation.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Missing SoB.

> ---
> Changes since v3:
> 
> - Rename structure from IPAFrameContext to FrameContext
> - Move changes to IPA modules to separate patches
> - Make the FrameContext::frame member private
> ---
>  src/ipa/libipa/fc_queue.cpp | 18 ++++++++++++++++++
>  src/ipa/libipa/fc_queue.h   | 10 ++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> index b81d497e255a..8f61c85131c9 100644
> --- a/src/ipa/libipa/fc_queue.cpp
> +++ b/src/ipa/libipa/fc_queue.cpp
> @@ -20,6 +20,24 @@ namespace ipa {
>   * \brief Queue to access per-frame Context
>   */
>  
> +/**
> + * \struct FrameContext
> + * \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 FrameContext::frame
> + * \brief The frame number
> + */
> +
>  /**
>   * \class FCQueue
>   * \brief A support class for queueing FrameContext instances through the IPA
> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> index 0f3af0f3a189..d8a5b8297a70 100644
> --- a/src/ipa/libipa/fc_queue.h
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -8,6 +8,7 @@
>  #pragma once
>  
>  #include <algorithm>
> +#include <stdint.h>
>  #include <vector>
>  
>  #include <libcamera/base/log.h>
> @@ -18,6 +19,15 @@ LOG_DECLARE_CATEGORY(FCQueue)
>  
>  namespace ipa {
>  
> +template<typename FrameContext>
> +class FCQueue;
> +
> +struct FrameContext {
> +private:
> +       template<typename T> friend class FCQueue;
> +       uint32_t frame;

Seems fine to me.

I'm still not sure what else is going to end up in here. There's a
chance that we might need to store the incoming Request ControlList (or
a copy of) ... so I still think there's sufficient requirement for the
common base type.


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

> +};
> +
>  template<typename FrameContext>
>  class FCQueue
>  {
> -- 
> Regards,
> 
> Laurent Pinchart
>

On Tue, Sep 20, 2022 at 02:40:28PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-09-08 02:41:33)
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > Provide a common FrameContext as a base for IPA modules to inherit from.
> > 
> > This will allow having a common set of parameters for every frame
> > context managed by the FCQueue implementation.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Missing SoB.
> > ---
> > Changes since v3:
> > 
> > - Rename structure from IPAFrameContext to FrameContext
> > - Move changes to IPA modules to separate patches
> > - Make the FrameContext::frame member private
> > ---
> >  src/ipa/libipa/fc_queue.cpp | 18 ++++++++++++++++++
> >  src/ipa/libipa/fc_queue.h   | 10 ++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > index b81d497e255a..8f61c85131c9 100644
> > --- a/src/ipa/libipa/fc_queue.cpp
> > +++ b/src/ipa/libipa/fc_queue.cpp
> > @@ -20,6 +20,24 @@ namespace ipa {
> >   * \brief Queue to access per-frame Context
> >   */
> >  
> > +/**
> > + * \struct FrameContext
> > + * \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 FrameContext::frame
> > + * \brief The frame number
> > + */
> > +
> >  /**
> >   * \class FCQueue
> >   * \brief A support class for queueing FrameContext instances through the IPA
> > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > index 0f3af0f3a189..d8a5b8297a70 100644
> > --- a/src/ipa/libipa/fc_queue.h
> > +++ b/src/ipa/libipa/fc_queue.h
> > @@ -8,6 +8,7 @@
> >  #pragma once
> >  
> >  #include <algorithm>
> > +#include <stdint.h>
> >  #include <vector>
> >  
> >  #include <libcamera/base/log.h>
> > @@ -18,6 +19,15 @@ LOG_DECLARE_CATEGORY(FCQueue)
> >  
> >  namespace ipa {
> >  
> > +template<typename FrameContext>
> > +class FCQueue;
> > +
> > +struct FrameContext {
> > +private:
> > +       template<typename T> friend class FCQueue;
> > +       uint32_t frame;
> 
> Seems fine to me.
> 
> I'm still not sure what else is going to end up in here. There's a
> chance that we might need to store the incoming Request ControlList (or
> a copy of) ... so I still think there's sufficient requirement for the
> common base type.

I haven't found a need for storing additional data in the base class in
the RkISP1 IPA. We'll see.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +};
> > +
> >  template<typename FrameContext>
> >  class FCQueue
> >  {

Hi Laurent

On Thu, Sep 08, 2022 at 04:41:33AM +0300, Laurent Pinchart via libcamera-devel wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> Provide a common FrameContext as a base for IPA modules to inherit from.
>
> This will allow having a common set of parameters for every frame
> context managed by the FCQueue implementation.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
> Changes since v3:
>
> - Rename structure from IPAFrameContext to FrameContext
> - Move changes to IPA modules to separate patches
> - Make the FrameContext::frame member private
> ---
>  src/ipa/libipa/fc_queue.cpp | 18 ++++++++++++++++++
>  src/ipa/libipa/fc_queue.h   | 10 ++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> index b81d497e255a..8f61c85131c9 100644
> --- a/src/ipa/libipa/fc_queue.cpp
> +++ b/src/ipa/libipa/fc_queue.cpp
> @@ -20,6 +20,24 @@ namespace ipa {
>   * \brief Queue to access per-frame Context
>   */
>
> +/**
> + * \struct FrameContext
> + * \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 FrameContext::frame
> + * \brief The frame number
> + */
> +
>  /**
>   * \class FCQueue
>   * \brief A support class for queueing FrameContext instances through the IPA
> diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> index 0f3af0f3a189..d8a5b8297a70 100644
> --- a/src/ipa/libipa/fc_queue.h
> +++ b/src/ipa/libipa/fc_queue.h
> @@ -8,6 +8,7 @@
>  #pragma once
>
>  #include <algorithm>
> +#include <stdint.h>
>  #include <vector>
>
>  #include <libcamera/base/log.h>
> @@ -18,6 +19,15 @@ LOG_DECLARE_CATEGORY(FCQueue)
>
>  namespace ipa {
>
> +template<typename FrameContext>
> +class FCQueue;
> +
> +struct FrameContext {
> +private:
> +	template<typename T> friend class FCQueue;
> +	uint32_t frame;
> +};
> +
>  template<typename FrameContext>
>  class FCQueue
>  {
> --
> Regards,
>
> Laurent Pinchart
>

Quoting Laurent Pinchart (2022-09-20 20:12:58)
> On Tue, Sep 20, 2022 at 02:40:28PM +0100, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2022-09-08 02:41:33)
> > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > Provide a common FrameContext as a base for IPA modules to inherit from.
> > > 
> > > This will allow having a common set of parameters for every frame
> > > context managed by the FCQueue implementation.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > Missing SoB.
> > > ---
> > > Changes since v3:
> > > 
> > > - Rename structure from IPAFrameContext to FrameContext
> > > - Move changes to IPA modules to separate patches
> > > - Make the FrameContext::frame member private
> > > ---
> > >  src/ipa/libipa/fc_queue.cpp | 18 ++++++++++++++++++
> > >  src/ipa/libipa/fc_queue.h   | 10 ++++++++++
> > >  2 files changed, 28 insertions(+)
> > > 
> > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > > index b81d497e255a..8f61c85131c9 100644
> > > --- a/src/ipa/libipa/fc_queue.cpp
> > > +++ b/src/ipa/libipa/fc_queue.cpp
> > > @@ -20,6 +20,24 @@ namespace ipa {
> > >   * \brief Queue to access per-frame Context
> > >   */
> > >  
> > > +/**
> > > + * \struct FrameContext
> > > + * \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 FrameContext::frame
> > > + * \brief The frame number
> > > + */
> > > +
> > >  /**
> > >   * \class FCQueue
> > >   * \brief A support class for queueing FrameContext instances through the IPA
> > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > > index 0f3af0f3a189..d8a5b8297a70 100644
> > > --- a/src/ipa/libipa/fc_queue.h
> > > +++ b/src/ipa/libipa/fc_queue.h
> > > @@ -8,6 +8,7 @@
> > >  #pragma once
> > >  
> > >  #include <algorithm>
> > > +#include <stdint.h>
> > >  #include <vector>
> > >  
> > >  #include <libcamera/base/log.h>
> > > @@ -18,6 +19,15 @@ LOG_DECLARE_CATEGORY(FCQueue)
> > >  
> > >  namespace ipa {
> > >  
> > > +template<typename FrameContext>
> > > +class FCQueue;
> > > +
> > > +struct FrameContext {
> > > +private:
> > > +       template<typename T> friend class FCQueue;
> > > +       uint32_t frame;
> > 
> > Seems fine to me.
> > 
> > I'm still not sure what else is going to end up in here. There's a
> > chance that we might need to store the incoming Request ControlList (or
> > a copy of) ... so I still think there's sufficient requirement for the
> > common base type.
> 
> I haven't found a need for storing additional data in the base class in
> the RkISP1 IPA. We'll see.

We should add a 
	ControlList metadata;

here in the base FrameContext. It can certainly be on top of this
series, and it should then get sent to the Pipeline handler to merge in
to the Request::metadata().

That should be common to all algortihms using this design, and the
algorithms can use it to populate the Request metadata at the earliest
opportunity.

--
Kieran


> 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > > +};
> > > +
> > >  template<typename FrameContext>
> > >  class FCQueue
> > >  {
> 
> -- 
> Regards,
> 
> Laurent Pinchart

On Thu, Sep 22, 2022 at 10:18:40AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-09-20 20:12:58)
> > On Tue, Sep 20, 2022 at 02:40:28PM +0100, Kieran Bingham wrote:
> > > Quoting Laurent Pinchart (2022-09-08 02:41:33)
> > > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > 
> > > > Provide a common FrameContext as a base for IPA modules to inherit from.
> > > > 
> > > > This will allow having a common set of parameters for every frame
> > > > context managed by the FCQueue implementation.
> > > > 
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > Missing SoB.
> > > 
> > > > ---
> > > > Changes since v3:
> > > > 
> > > > - Rename structure from IPAFrameContext to FrameContext
> > > > - Move changes to IPA modules to separate patches
> > > > - Make the FrameContext::frame member private
> > > > ---
> > > >  src/ipa/libipa/fc_queue.cpp | 18 ++++++++++++++++++
> > > >  src/ipa/libipa/fc_queue.h   | 10 ++++++++++
> > > >  2 files changed, 28 insertions(+)
> > > > 
> > > > diff --git a/src/ipa/libipa/fc_queue.cpp b/src/ipa/libipa/fc_queue.cpp
> > > > index b81d497e255a..8f61c85131c9 100644
> > > > --- a/src/ipa/libipa/fc_queue.cpp
> > > > +++ b/src/ipa/libipa/fc_queue.cpp
> > > > @@ -20,6 +20,24 @@ namespace ipa {
> > > >   * \brief Queue to access per-frame Context
> > > >   */
> > > >  
> > > > +/**
> > > > + * \struct FrameContext
> > > > + * \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 FrameContext::frame
> > > > + * \brief The frame number
> > > > + */
> > > > +
> > > >  /**
> > > >   * \class FCQueue
> > > >   * \brief A support class for queueing FrameContext instances through the IPA
> > > > diff --git a/src/ipa/libipa/fc_queue.h b/src/ipa/libipa/fc_queue.h
> > > > index 0f3af0f3a189..d8a5b8297a70 100644
> > > > --- a/src/ipa/libipa/fc_queue.h
> > > > +++ b/src/ipa/libipa/fc_queue.h
> > > > @@ -8,6 +8,7 @@
> > > >  #pragma once
> > > >  
> > > >  #include <algorithm>
> > > > +#include <stdint.h>
> > > >  #include <vector>
> > > >  
> > > >  #include <libcamera/base/log.h>
> > > > @@ -18,6 +19,15 @@ LOG_DECLARE_CATEGORY(FCQueue)
> > > >  
> > > >  namespace ipa {
> > > >  
> > > > +template<typename FrameContext>
> > > > +class FCQueue;
> > > > +
> > > > +struct FrameContext {
> > > > +private:
> > > > +       template<typename T> friend class FCQueue;
> > > > +       uint32_t frame;
> > > 
> > > Seems fine to me.
> > > 
> > > I'm still not sure what else is going to end up in here. There's a
> > > chance that we might need to store the incoming Request ControlList (or
> > > a copy of) ... so I still think there's sufficient requirement for the
> > > common base type.
> > 
> > I haven't found a need for storing additional data in the base class in
> > the RkISP1 IPA. We'll see.
> 
> We should add a 
> 	ControlList metadata;
> 
> here in the base FrameContext. It can certainly be on top of this
> series, and it should then get sent to the Pipeline handler to merge in
> to the Request::metadata().

It's a good idea, but I wonder how we can achieve that given that the
ControlList should be constructed with a camera-specific ControlInfoMap.

> That should be common to all algortihms using this design, and the
> algorithms can use it to populate the Request metadata at the earliest
> opportunity.
> 
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > 
> > > > +};
> > > > +
> > > >  template<typename FrameContext>
> > > >  class FCQueue
> > > >  {