[4/5] ipa: rkisp1: blc: Report sensor black levels in metadata
diff mbox series

Message ID 20240701144122.3418955-5-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: Add black level to camera sensor helpers
Related show

Commit Message

Stefan Klug July 1, 2024, 2:38 p.m. UTC 
For the tuning process it is necessary to know the sensor black levels.
Add them to the metadata.

Additionally enable raw support for this algorithm and add it to
uncalibrated.yaml, so that black levels get reported when capturing
tuning images.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/blc.cpp     | 19 +++++++++++++++++++
 src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
 src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
 3 files changed, 24 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi July 2, 2024, 8:45 a.m. UTC | #1 
Hi Stefan

On Mon, Jul 01, 2024 at 04:38:27PM GMT, Stefan Klug wrote:
> For the tuning process it is necessary to know the sensor black levels.
> Add them to the metadata.
>
> Additionally enable raw support for this algorithm and add it to
> uncalibrated.yaml, so that black levels get reported when capturing
> tuning images.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/blc.cpp     | 19 +++++++++++++++++++
>  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
>  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
>  3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> index 0c39c3b47da5..a9f76b87d15a 100644
> --- a/src/ipa/rkisp1/algorithms/blc.cpp
> +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> @@ -9,6 +9,8 @@
>
>  #include <libcamera/base/log.h>
>
> +#include <libcamera/control_ids.h>
> +
>  #include "libcamera/internal/yaml_parser.h"
>
>  /**
> @@ -38,6 +40,7 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
>  BlackLevelCorrection::BlackLevelCorrection()
>  	: tuningParameters_(false)
>  {
> +	supportsRaw_ = true;

Does it support raw for real ? Doesn't BLC require going through the
ISP ?

>  }
>
>  /**
> @@ -107,6 +110,22 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
>  	params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_BLS;
>  }
>
> +/**
> + * \copydoc libcamera::ipa::Algorithm::process
> + */
> +void BlackLevelCorrection::process([[maybe_unused]] IPAContext &context,
> +				   [[maybe_unused]] const uint32_t frame,
> +				   [[maybe_unused]] IPAFrameContext &frameContext,
> +				   [[maybe_unused]] const rkisp1_stat_buffer *stats,
> +				   [[maybe_unused]] ControlList &metadata)
> +{
> +	metadata.set(controls::SensorBlackLevels,
> +		     { static_cast<int32_t>(blackLevelRed_),
> +		       static_cast<int32_t>(blackLevelGreenR_),
> +		       static_cast<int32_t>(blackLevelGreenB_),
> +		       static_cast<int32_t>(blackLevelBlue_) });

As black levels do not change you can store them in the context and
populate the metadata in the main IPA module instead of making this
algorithm support raw ?

> +}
> +
>  REGISTER_IPA_ALGORITHM(BlackLevelCorrection, "BlackLevelCorrection")
>
>  } /* namespace ipa::rkisp1::algorithms */
> diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h
> index 460ebcc15739..4ecac233f88b 100644
> --- a/src/ipa/rkisp1/algorithms/blc.h
> +++ b/src/ipa/rkisp1/algorithms/blc.h
> @@ -23,7 +23,10 @@ public:
>  	void prepare(IPAContext &context, const uint32_t frame,
>  		     IPAFrameContext &frameContext,
>  		     rkisp1_params_cfg *params) override;
> -
> +	void process(IPAContext &context, const uint32_t frame,
> +		     IPAFrameContext &frameContext,
> +		     const rkisp1_stat_buffer *stats,
> +		     ControlList &metadata) override;
>  private:
>  	bool tuningParameters_;
>  	int16_t blackLevelRed_;
> diff --git a/src/ipa/rkisp1/data/uncalibrated.yaml b/src/ipa/rkisp1/data/uncalibrated.yaml
> index a7bbd8d84263..609012967e02 100644
> --- a/src/ipa/rkisp1/data/uncalibrated.yaml
> +++ b/src/ipa/rkisp1/data/uncalibrated.yaml
> @@ -5,4 +5,5 @@ version: 1
>  algorithms:
>    - Agc:
>    - Awb:
> +  - BlackLevelCorrection:
>  ...
> --
> 2.43.0
>
Kieran Bingham July 2, 2024, 11:49 a.m. UTC | #2 
Quoting Jacopo Mondi (2024-07-02 09:45:26)
> Hi Stefan
> 
> On Mon, Jul 01, 2024 at 04:38:27PM GMT, Stefan Klug wrote:
> > For the tuning process it is necessary to know the sensor black levels.
> > Add them to the metadata.
> >
> > Additionally enable raw support for this algorithm and add it to
> > uncalibrated.yaml, so that black levels get reported when capturing
> > tuning images.
> >
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/blc.cpp     | 19 +++++++++++++++++++
> >  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
> >  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
> >  3 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > index 0c39c3b47da5..a9f76b87d15a 100644
> > --- a/src/ipa/rkisp1/algorithms/blc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> > @@ -9,6 +9,8 @@
> >
> >  #include <libcamera/base/log.h>
> >
> > +#include <libcamera/control_ids.h>
> > +
> >  #include "libcamera/internal/yaml_parser.h"
> >
> >  /**
> > @@ -38,6 +40,7 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
> >  BlackLevelCorrection::BlackLevelCorrection()
> >       : tuningParameters_(false)
> >  {
> > +     supportsRaw_ = true;
> 
> Does it support raw for real ? Doesn't BLC require going through the
> ISP ?

Oh That's interesting, - I didn't realise we had this switch

Any preference for adding it in the constructor rather than extending
the initiasliser list and putting it after tuningParameters_(false) ?

>
> >  }
> >
> >  /**
> > @@ -107,6 +110,22 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> >       params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_BLS;
> >  }
> >
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::process
> > + */
> > +void BlackLevelCorrection::process([[maybe_unused]] IPAContext &context,
> > +                                [[maybe_unused]] const uint32_t frame,
> > +                                [[maybe_unused]] IPAFrameContext &frameContext,
> > +                                [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > +                                [[maybe_unused]] ControlList &metadata)
> > +{
> > +     metadata.set(controls::SensorBlackLevels,
> > +                  { static_cast<int32_t>(blackLevelRed_),
> > +                    static_cast<int32_t>(blackLevelGreenR_),
> > +                    static_cast<int32_t>(blackLevelGreenB_),
> > +                    static_cast<int32_t>(blackLevelBlue_) });
> 
> As black levels do not change you can store them in the context and
> populate the metadata in the main IPA module instead of making this
> algorithm support raw ?


If a sensor exposes a control to allow setting the black level, I expect
this component would be responsible for reporting that and then setting
it to the v4l2-subdev driver?

> 
> > +}
> > +
> >  REGISTER_IPA_ALGORITHM(BlackLevelCorrection, "BlackLevelCorrection")
> >
> >  } /* namespace ipa::rkisp1::algorithms */
> > diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h
> > index 460ebcc15739..4ecac233f88b 100644
> > --- a/src/ipa/rkisp1/algorithms/blc.h
> > +++ b/src/ipa/rkisp1/algorithms/blc.h
> > @@ -23,7 +23,10 @@ public:
> >       void prepare(IPAContext &context, const uint32_t frame,
> >                    IPAFrameContext &frameContext,
> >                    rkisp1_params_cfg *params) override;
> > -
> > +     void process(IPAContext &context, const uint32_t frame,
> > +                  IPAFrameContext &frameContext,
> > +                  const rkisp1_stat_buffer *stats,
> > +                  ControlList &metadata) override;
> >  private:
> >       bool tuningParameters_;
> >       int16_t blackLevelRed_;
> > diff --git a/src/ipa/rkisp1/data/uncalibrated.yaml b/src/ipa/rkisp1/data/uncalibrated.yaml
> > index a7bbd8d84263..609012967e02 100644
> > --- a/src/ipa/rkisp1/data/uncalibrated.yaml
> > +++ b/src/ipa/rkisp1/data/uncalibrated.yaml
> > @@ -5,4 +5,5 @@ version: 1
> >  algorithms:
> >    - Agc:
> >    - Awb:
> > +  - BlackLevelCorrection:
> >  ...
> > --
> > 2.43.0
> >
Stefan Klug July 2, 2024, 1:45 p.m. UTC | #3 
Hi Jacopo,

On Tue, Jul 02, 2024 at 10:45:26AM +0200, Jacopo Mondi wrote:
> Hi Stefan
> 
> On Mon, Jul 01, 2024 at 04:38:27PM GMT, Stefan Klug wrote:
> > For the tuning process it is necessary to know the sensor black levels.
> > Add them to the metadata.
> >
> > Additionally enable raw support for this algorithm and add it to
> > uncalibrated.yaml, so that black levels get reported when capturing
> > tuning images.
> >
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/blc.cpp     | 19 +++++++++++++++++++
> >  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
> >  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
> >  3 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > index 0c39c3b47da5..a9f76b87d15a 100644
> > --- a/src/ipa/rkisp1/algorithms/blc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> > @@ -9,6 +9,8 @@
> >
> >  #include <libcamera/base/log.h>
> >
> > +#include <libcamera/control_ids.h>
> > +
> >  #include "libcamera/internal/yaml_parser.h"
> >
> >  /**
> > @@ -38,6 +40,7 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
> >  BlackLevelCorrection::BlackLevelCorrection()
> >  	: tuningParameters_(false)
> >  {
> > +	supportsRaw_ = true;
> 
> Does it support raw for real ? Doesn't BLC require going through the
> ISP ?

Hm good question. What I wanted to achieve:
- black level data should be contained in the metadata so that we can
  capture raws with blacklevel
- It should be possible to overwrite the values with a tuning file, for
  cases, where you take the tuning files with a libcamera that doesn't
  know the correct black level for that sensor (it's arguable if that's
  an acceptable usecase, but I think it happens in practice)

So I could either move the whole configure logic into the ipa core, or
add raw support to the blc algorithm. To me it feels better to keep all
the black level related stuff in one place. But I'm ok with changing
that.

> 
> >  }
> >
> >  /**
> > @@ -107,6 +110,22 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> >  	params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_BLS;
> >  }
> >
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::process
> > + */
> > +void BlackLevelCorrection::process([[maybe_unused]] IPAContext &context,
> > +				   [[maybe_unused]] const uint32_t frame,
> > +				   [[maybe_unused]] IPAFrameContext &frameContext,
> > +				   [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > +				   [[maybe_unused]] ControlList &metadata)
> > +{
> > +	metadata.set(controls::SensorBlackLevels,
> > +		     { static_cast<int32_t>(blackLevelRed_),
> > +		       static_cast<int32_t>(blackLevelGreenR_),
> > +		       static_cast<int32_t>(blackLevelGreenB_),
> > +		       static_cast<int32_t>(blackLevelBlue_) });
> 
> As black levels do not change you can store them in the context and
> populate the metadata in the main IPA module instead of making this
> algorithm support raw ?

Just to clarify: That also means the yaml reading code should be moved
there, right?

Best regards,
Stefan

> 
> > +}
> > +
> >  REGISTER_IPA_ALGORITHM(BlackLevelCorrection, "BlackLevelCorrection")
> >
> >  } /* namespace ipa::rkisp1::algorithms */
> > diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h
> > index 460ebcc15739..4ecac233f88b 100644
> > --- a/src/ipa/rkisp1/algorithms/blc.h
> > +++ b/src/ipa/rkisp1/algorithms/blc.h
> > @@ -23,7 +23,10 @@ public:
> >  	void prepare(IPAContext &context, const uint32_t frame,
> >  		     IPAFrameContext &frameContext,
> >  		     rkisp1_params_cfg *params) override;
> > -
> > +	void process(IPAContext &context, const uint32_t frame,
> > +		     IPAFrameContext &frameContext,
> > +		     const rkisp1_stat_buffer *stats,
> > +		     ControlList &metadata) override;
> >  private:
> >  	bool tuningParameters_;
> >  	int16_t blackLevelRed_;
> > diff --git a/src/ipa/rkisp1/data/uncalibrated.yaml b/src/ipa/rkisp1/data/uncalibrated.yaml
> > index a7bbd8d84263..609012967e02 100644
> > --- a/src/ipa/rkisp1/data/uncalibrated.yaml
> > +++ b/src/ipa/rkisp1/data/uncalibrated.yaml
> > @@ -5,4 +5,5 @@ version: 1
> >  algorithms:
> >    - Agc:
> >    - Awb:
> > +  - BlackLevelCorrection:
> >  ...
> > --
> > 2.43.0
> >
Stefan Klug July 2, 2024, 1:48 p.m. UTC | #4 
On Tue, Jul 02, 2024 at 12:49:59PM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2024-07-02 09:45:26)
> > Hi Stefan
> > 
> > On Mon, Jul 01, 2024 at 04:38:27PM GMT, Stefan Klug wrote:
> > > For the tuning process it is necessary to know the sensor black levels.
> > > Add them to the metadata.
> > >
> > > Additionally enable raw support for this algorithm and add it to
> > > uncalibrated.yaml, so that black levels get reported when capturing
> > > tuning images.
> > >
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/blc.cpp     | 19 +++++++++++++++++++
> > >  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
> > >  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
> > >  3 files changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > > index 0c39c3b47da5..a9f76b87d15a 100644
> > > --- a/src/ipa/rkisp1/algorithms/blc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> > > @@ -9,6 +9,8 @@
> > >
> > >  #include <libcamera/base/log.h>
> > >
> > > +#include <libcamera/control_ids.h>
> > > +
> > >  #include "libcamera/internal/yaml_parser.h"
> > >
> > >  /**
> > > @@ -38,6 +40,7 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
> > >  BlackLevelCorrection::BlackLevelCorrection()
> > >       : tuningParameters_(false)
> > >  {
> > > +     supportsRaw_ = true;
> > 
> > Does it support raw for real ? Doesn't BLC require going through the
> > ISP ?
> 
> Oh That's interesting, - I didn't realise we had this switch
> 
> Any preference for adding it in the constructor rather than extending
> the initiasliser list and putting it after tuningParameters_(false) ?

Actually I looked that up, because I had that question myself.
supportsRaw_ is defined in the base class. The initializer list can only
initialize variables from the current class. So assignments in the
constructor are the only option.

> 
> >
> > >  }
> > >
> > >  /**
> > > @@ -107,6 +110,22 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> > >       params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_BLS;
> > >  }
> > >
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::process
> > > + */
> > > +void BlackLevelCorrection::process([[maybe_unused]] IPAContext &context,
> > > +                                [[maybe_unused]] const uint32_t frame,
> > > +                                [[maybe_unused]] IPAFrameContext &frameContext,
> > > +                                [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > +                                [[maybe_unused]] ControlList &metadata)
> > > +{
> > > +     metadata.set(controls::SensorBlackLevels,
> > > +                  { static_cast<int32_t>(blackLevelRed_),
> > > +                    static_cast<int32_t>(blackLevelGreenR_),
> > > +                    static_cast<int32_t>(blackLevelGreenB_),
> > > +                    static_cast<int32_t>(blackLevelBlue_) });
> > 
> > As black levels do not change you can store them in the context and
> > populate the metadata in the main IPA module instead of making this
> > algorithm support raw ?
> 
> 
> If a sensor exposes a control to allow setting the black level, I expect
> this component would be responsible for reporting that and then setting
> it to the v4l2-subdev driver?

Yes, if we ever implement such a driver :-)

Best regards,
Stefan

> 
> > 
> > > +}
> > > +
> > >  REGISTER_IPA_ALGORITHM(BlackLevelCorrection, "BlackLevelCorrection")
> > >
> > >  } /* namespace ipa::rkisp1::algorithms */
> > > diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h
> > > index 460ebcc15739..4ecac233f88b 100644
> > > --- a/src/ipa/rkisp1/algorithms/blc.h
> > > +++ b/src/ipa/rkisp1/algorithms/blc.h
> > > @@ -23,7 +23,10 @@ public:
> > >       void prepare(IPAContext &context, const uint32_t frame,
> > >                    IPAFrameContext &frameContext,
> > >                    rkisp1_params_cfg *params) override;
> > > -
> > > +     void process(IPAContext &context, const uint32_t frame,
> > > +                  IPAFrameContext &frameContext,
> > > +                  const rkisp1_stat_buffer *stats,
> > > +                  ControlList &metadata) override;
> > >  private:
> > >       bool tuningParameters_;
> > >       int16_t blackLevelRed_;
> > > diff --git a/src/ipa/rkisp1/data/uncalibrated.yaml b/src/ipa/rkisp1/data/uncalibrated.yaml
> > > index a7bbd8d84263..609012967e02 100644
> > > --- a/src/ipa/rkisp1/data/uncalibrated.yaml
> > > +++ b/src/ipa/rkisp1/data/uncalibrated.yaml
> > > @@ -5,4 +5,5 @@ version: 1
> > >  algorithms:
> > >    - Agc:
> > >    - Awb:
> > > +  - BlackLevelCorrection:
> > >  ...
> > > --
> > > 2.43.0
> > >
Laurent Pinchart July 2, 2024, 2:39 p.m. UTC | #5 
On Tue, Jul 02, 2024 at 03:45:16PM +0200, Stefan Klug wrote:
> On Tue, Jul 02, 2024 at 10:45:26AM +0200, Jacopo Mondi wrote:
> > On Mon, Jul 01, 2024 at 04:38:27PM GMT, Stefan Klug wrote:
> > > For the tuning process it is necessary to know the sensor black levels.
> > > Add them to the metadata.
> > >
> > > Additionally enable raw support for this algorithm and add it to
> > > uncalibrated.yaml, so that black levels get reported when capturing
> > > tuning images.
> > >
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/blc.cpp     | 19 +++++++++++++++++++
> > >  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
> > >  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
> > >  3 files changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > > index 0c39c3b47da5..a9f76b87d15a 100644
> > > --- a/src/ipa/rkisp1/algorithms/blc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> > > @@ -9,6 +9,8 @@
> > >
> > >  #include <libcamera/base/log.h>
> > >
> > > +#include <libcamera/control_ids.h>
> > > +
> > >  #include "libcamera/internal/yaml_parser.h"
> > >
> > >  /**
> > > @@ -38,6 +40,7 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
> > >  BlackLevelCorrection::BlackLevelCorrection()
> > >  	: tuningParameters_(false)
> > >  {
> > > +	supportsRaw_ = true;
> > 
> > Does it support raw for real ? Doesn't BLC require going through the
> > ISP ?
> 
> Hm good question. What I wanted to achieve:
> - black level data should be contained in the metadata so that we can
>   capture raws with blacklevel
> - It should be possible to overwrite the values with a tuning file, for
>   cases, where you take the tuning files with a libcamera that doesn't
>   know the correct black level for that sensor (it's arguable if that's
>   an acceptable usecase, but I think it happens in practice)
> 
> So I could either move the whole configure logic into the ipa core, or
> add raw support to the blc algorithm. To me it feels better to keep all
> the black level related stuff in one place. But I'm ok with changing
> that.

Supporting raw here sounds easier indeed, even if it may be cheating a
bit. I'm fine with it, but a comment that explains what's going on would
be useful. You probably want to update the prepare() function to return
immediately when capturing raw, as programming the ISP makes little
sense in that case.

> > >  }
> > >
> > >  /**
> > > @@ -107,6 +110,22 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> > >  	params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_BLS;
> > >  }
> > >
> > > +/**
> > > + * \copydoc libcamera::ipa::Algorithm::process
> > > + */
> > > +void BlackLevelCorrection::process([[maybe_unused]] IPAContext &context,
> > > +				   [[maybe_unused]] const uint32_t frame,
> > > +				   [[maybe_unused]] IPAFrameContext &frameContext,
> > > +				   [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > +				   [[maybe_unused]] ControlList &metadata)
> > > +{
> > > +	metadata.set(controls::SensorBlackLevels,
> > > +		     { static_cast<int32_t>(blackLevelRed_),
> > > +		       static_cast<int32_t>(blackLevelGreenR_),
> > > +		       static_cast<int32_t>(blackLevelGreenB_),
> > > +		       static_cast<int32_t>(blackLevelBlue_) });
> > 
> > As black levels do not change you can store them in the context and
> > populate the metadata in the main IPA module instead of making this
> > algorithm support raw ?
> 
> Just to clarify: That also means the yaml reading code should be moved
> there, right?

There are drawbacks to enabling raw support in this algorithms (it's a
bit of a hack), and moving the code to the IPA module top-level code
(it's a bit of a hack). I think this hack is cleaner.

> > > +}
> > > +
> > >  REGISTER_IPA_ALGORITHM(BlackLevelCorrection, "BlackLevelCorrection")
> > >
> > >  } /* namespace ipa::rkisp1::algorithms */
> > > diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h
> > > index 460ebcc15739..4ecac233f88b 100644
> > > --- a/src/ipa/rkisp1/algorithms/blc.h
> > > +++ b/src/ipa/rkisp1/algorithms/blc.h
> > > @@ -23,7 +23,10 @@ public:
> > >  	void prepare(IPAContext &context, const uint32_t frame,
> > >  		     IPAFrameContext &frameContext,
> > >  		     rkisp1_params_cfg *params) override;
> > > -
> > > +	void process(IPAContext &context, const uint32_t frame,
> > > +		     IPAFrameContext &frameContext,
> > > +		     const rkisp1_stat_buffer *stats,
> > > +		     ControlList &metadata) override;
> > >  private:
> > >  	bool tuningParameters_;
> > >  	int16_t blackLevelRed_;
> > > diff --git a/src/ipa/rkisp1/data/uncalibrated.yaml b/src/ipa/rkisp1/data/uncalibrated.yaml
> > > index a7bbd8d84263..609012967e02 100644
> > > --- a/src/ipa/rkisp1/data/uncalibrated.yaml
> > > +++ b/src/ipa/rkisp1/data/uncalibrated.yaml
> > > @@ -5,4 +5,5 @@ version: 1
> > >  algorithms:
> > >    - Agc:
> > >    - Awb:
> > > +  - BlackLevelCorrection:
> > >  ...
Paul Elder July 3, 2024, 11:19 a.m. UTC | #6 
On Tue, Jul 02, 2024 at 05:39:56PM +0300, Laurent Pinchart wrote:
> On Tue, Jul 02, 2024 at 03:45:16PM +0200, Stefan Klug wrote:
> > On Tue, Jul 02, 2024 at 10:45:26AM +0200, Jacopo Mondi wrote:
> > > On Mon, Jul 01, 2024 at 04:38:27PM GMT, Stefan Klug wrote:
> > > > For the tuning process it is necessary to know the sensor black levels.
> > > > Add them to the metadata.
> > > >
> > > > Additionally enable raw support for this algorithm and add it to
> > > > uncalibrated.yaml, so that black levels get reported when capturing
> > > > tuning images.
> > > >
> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > ---
> > > >  src/ipa/rkisp1/algorithms/blc.cpp     | 19 +++++++++++++++++++
> > > >  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
> > > >  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
> > > >  3 files changed, 24 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > > > index 0c39c3b47da5..a9f76b87d15a 100644
> > > > --- a/src/ipa/rkisp1/algorithms/blc.cpp
> > > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> > > > @@ -9,6 +9,8 @@
> > > >
> > > >  #include <libcamera/base/log.h>
> > > >
> > > > +#include <libcamera/control_ids.h>
> > > > +
> > > >  #include "libcamera/internal/yaml_parser.h"
> > > >
> > > >  /**
> > > > @@ -38,6 +40,7 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
> > > >  BlackLevelCorrection::BlackLevelCorrection()
> > > >  	: tuningParameters_(false)
> > > >  {
> > > > +	supportsRaw_ = true;
> > > 
> > > Does it support raw for real ? Doesn't BLC require going through the
> > > ISP ?
> > 
> > Hm good question. What I wanted to achieve:
> > - black level data should be contained in the metadata so that we can
> >   capture raws with blacklevel

Ah I see so this is the one that's mainly causing the hack.

I was going to say yeah I didn't think that blc works without the ISP.

> > - It should be possible to overwrite the values with a tuning file, for
> >   cases, where you take the tuning files with a libcamera that doesn't
> >   know the correct black level for that sensor (it's arguable if that's
> >   an acceptable usecase, but I think it happens in practice)
> > 
> > So I could either move the whole configure logic into the ipa core, or
> > add raw support to the blc algorithm. To me it feels better to keep all
> > the black level related stuff in one place. But I'm ok with changing
> > that.
> 
> Supporting raw here sounds easier indeed, even if it may be cheating a
> bit. I'm fine with it, but a comment that explains what's going on would
> be useful. You probably want to update the prepare() function to return
> immediately when capturing raw, as programming the ISP makes little
> sense in that case.

+1


Paul

> 
> > > >  }
> > > >
> > > >  /**
> > > > @@ -107,6 +110,22 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> > > >  	params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_BLS;
> > > >  }
> > > >
> > > > +/**
> > > > + * \copydoc libcamera::ipa::Algorithm::process
> > > > + */
> > > > +void BlackLevelCorrection::process([[maybe_unused]] IPAContext &context,
> > > > +				   [[maybe_unused]] const uint32_t frame,
> > > > +				   [[maybe_unused]] IPAFrameContext &frameContext,
> > > > +				   [[maybe_unused]] const rkisp1_stat_buffer *stats,
> > > > +				   [[maybe_unused]] ControlList &metadata)
> > > > +{
> > > > +	metadata.set(controls::SensorBlackLevels,
> > > > +		     { static_cast<int32_t>(blackLevelRed_),
> > > > +		       static_cast<int32_t>(blackLevelGreenR_),
> > > > +		       static_cast<int32_t>(blackLevelGreenB_),
> > > > +		       static_cast<int32_t>(blackLevelBlue_) });
> > > 
> > > As black levels do not change you can store them in the context and
> > > populate the metadata in the main IPA module instead of making this
> > > algorithm support raw ?
> > 
> > Just to clarify: That also means the yaml reading code should be moved
> > there, right?
> 
> There are drawbacks to enabling raw support in this algorithms (it's a
> bit of a hack), and moving the code to the IPA module top-level code
> (it's a bit of a hack). I think this hack is cleaner.
> 
> > > > +}
> > > > +
> > > >  REGISTER_IPA_ALGORITHM(BlackLevelCorrection, "BlackLevelCorrection")
> > > >
> > > >  } /* namespace ipa::rkisp1::algorithms */
> > > > diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h
> > > > index 460ebcc15739..4ecac233f88b 100644
> > > > --- a/src/ipa/rkisp1/algorithms/blc.h
> > > > +++ b/src/ipa/rkisp1/algorithms/blc.h
> > > > @@ -23,7 +23,10 @@ public:
> > > >  	void prepare(IPAContext &context, const uint32_t frame,
> > > >  		     IPAFrameContext &frameContext,
> > > >  		     rkisp1_params_cfg *params) override;
> > > > -
> > > > +	void process(IPAContext &context, const uint32_t frame,
> > > > +		     IPAFrameContext &frameContext,
> > > > +		     const rkisp1_stat_buffer *stats,
> > > > +		     ControlList &metadata) override;
> > > >  private:
> > > >  	bool tuningParameters_;
> > > >  	int16_t blackLevelRed_;
> > > > diff --git a/src/ipa/rkisp1/data/uncalibrated.yaml b/src/ipa/rkisp1/data/uncalibrated.yaml
> > > > index a7bbd8d84263..609012967e02 100644
> > > > --- a/src/ipa/rkisp1/data/uncalibrated.yaml
> > > > +++ b/src/ipa/rkisp1/data/uncalibrated.yaml
> > > > @@ -5,4 +5,5 @@ version: 1
> > > >  algorithms:
> > > >    - Agc:
> > > >    - Awb:
> > > > +  - BlackLevelCorrection:
> > > >  ...
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
index 0c39c3b47da5..a9f76b87d15a 100644
--- a/src/ipa/rkisp1/algorithms/blc.cpp
+++ b/src/ipa/rkisp1/algorithms/blc.cpp
@@ -9,6 +9,8 @@ 
 
 #include <libcamera/base/log.h>
 
+#include <libcamera/control_ids.h>
+
 #include "libcamera/internal/yaml_parser.h"
 
 /**
@@ -38,6 +40,7 @@  LOG_DEFINE_CATEGORY(RkISP1Blc)
 BlackLevelCorrection::BlackLevelCorrection()
 	: tuningParameters_(false)
 {
+	supportsRaw_ = true;
 }
 
 /**
@@ -107,6 +110,22 @@  void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
 	params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_BLS;
 }
 
+/**
+ * \copydoc libcamera::ipa::Algorithm::process
+ */
+void BlackLevelCorrection::process([[maybe_unused]] IPAContext &context,
+				   [[maybe_unused]] const uint32_t frame,
+				   [[maybe_unused]] IPAFrameContext &frameContext,
+				   [[maybe_unused]] const rkisp1_stat_buffer *stats,
+				   [[maybe_unused]] ControlList &metadata)
+{
+	metadata.set(controls::SensorBlackLevels,
+		     { static_cast<int32_t>(blackLevelRed_),
+		       static_cast<int32_t>(blackLevelGreenR_),
+		       static_cast<int32_t>(blackLevelGreenB_),
+		       static_cast<int32_t>(blackLevelBlue_) });
+}
+
 REGISTER_IPA_ALGORITHM(BlackLevelCorrection, "BlackLevelCorrection")
 
 } /* namespace ipa::rkisp1::algorithms */
diff --git a/src/ipa/rkisp1/algorithms/blc.h b/src/ipa/rkisp1/algorithms/blc.h
index 460ebcc15739..4ecac233f88b 100644
--- a/src/ipa/rkisp1/algorithms/blc.h
+++ b/src/ipa/rkisp1/algorithms/blc.h
@@ -23,7 +23,10 @@  public:
 	void prepare(IPAContext &context, const uint32_t frame,
 		     IPAFrameContext &frameContext,
 		     rkisp1_params_cfg *params) override;
-
+	void process(IPAContext &context, const uint32_t frame,
+		     IPAFrameContext &frameContext,
+		     const rkisp1_stat_buffer *stats,
+		     ControlList &metadata) override;
 private:
 	bool tuningParameters_;
 	int16_t blackLevelRed_;
diff --git a/src/ipa/rkisp1/data/uncalibrated.yaml b/src/ipa/rkisp1/data/uncalibrated.yaml
index a7bbd8d84263..609012967e02 100644
--- a/src/ipa/rkisp1/data/uncalibrated.yaml
+++ b/src/ipa/rkisp1/data/uncalibrated.yaml
@@ -5,4 +5,5 @@  version: 1
 algorithms:
   - Agc:
   - Awb:
+  - BlackLevelCorrection:
 ...