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

Message ID 20240703104004.184783-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 3, 2024, 10:39 a.m. UTC 
Add sensor black levels to the metadata of the rkisp1 pipeline.

Additionally enable raw support for this algorithm and add it to
uncalibrated.yaml, so that black levels get reported when capturing
tuning images. This is a bit of a hack, because no actual black level
correction is taking place in raw mode, but it is the easiest way to get
blacklevel reported for raw streams.

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

Comments

Paul Elder July 3, 2024, 11:29 a.m. UTC | #1 
On Wed, Jul 03, 2024 at 12:39:51PM +0200, Stefan Klug wrote:
> Add sensor black levels to the metadata of the rkisp1 pipeline.
> 
> Additionally enable raw support for this algorithm and add it to
> uncalibrated.yaml, so that black levels get reported when capturing
> tuning images. This is a bit of a hack, because no actual black level
> correction is taking place in raw mode, but it is the easiest way to get
> blacklevel reported for raw streams.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/ipa/rkisp1/algorithms/blc.cpp     | 28 +++++++++++++++++++++++++++
>  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
>  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> index 87025e4f8c72..71c62b009707 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,13 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
>  BlackLevelCorrection::BlackLevelCorrection()
>  	: tuningParameters_(false)
>  {
> +	/*
> +	 * This is a bit of a hack. In raw mode no black level correction
> +	 * happens. This flag is used to ensure the metadata gets populated with
> +	 * the black level which is needed to capture proper raw images for
> +	 * tuning.
> +	 */
> +	supportsRaw_ = true;
>  }
>  
>  /**
> @@ -107,6 +116,9 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
>  				   [[maybe_unused]] IPAFrameContext &frameContext,
>  				   rkisp1_params_cfg *params)
>  {
> +	if (context.configuration.raw)
> +		return;
> +
>  	if (frame > 0)
>  		return;
>  
> @@ -125,6 +137,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:
>  ...
> -- 
> 2.43.0
>
Kieran Bingham July 3, 2024, 12:15 p.m. UTC | #2 
Quoting Stefan Klug (2024-07-03 11:39:51)
> Add sensor black levels to the metadata of the rkisp1 pipeline.
> 
> Additionally enable raw support for this algorithm and add it to
> uncalibrated.yaml, so that black levels get reported when capturing
> tuning images. This is a bit of a hack, because no actual black level
> correction is taking place in raw mode, but it is the easiest way to get
> blacklevel reported for raw streams.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/blc.cpp     | 28 +++++++++++++++++++++++++++
>  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
>  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
>  3 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> index 87025e4f8c72..71c62b009707 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,13 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
>  BlackLevelCorrection::BlackLevelCorrection()
>         : tuningParameters_(false)
>  {
> +       /*
> +        * This is a bit of a hack. In raw mode no black level correction
> +        * happens. This flag is used to ensure the metadata gets populated with
> +        * the black level which is needed to capture proper raw images for
> +        * tuning.
> +        */
> +       supportsRaw_ = true;
>  }
>  
>  /**
> @@ -107,6 +116,9 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
>                                    [[maybe_unused]] IPAFrameContext &frameContext,
>                                    rkisp1_params_cfg *params)
>  {
> +       if (context.configuration.raw)
> +               return;
> +
>         if (frame > 0)
>                 return;
>  
> @@ -125,6 +137,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)
> +{

Hrm. ... now ... should we report SensorBlackLevels at a bit-depth that
matches the bit-depth of the image?

I.e ... do we need to scale these 16 bit numbers to 8,10,12 bit
depending on the sensor configuration and output file bit-depth?

> +       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:
>  ...
> -- 
> 2.43.0
>
Stefan Klug July 3, 2024, 12:22 p.m. UTC | #3 
Hi Kieran,

On Wed, Jul 03, 2024 at 01:15:55PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-07-03 11:39:51)
> > Add sensor black levels to the metadata of the rkisp1 pipeline.
> > 
> > Additionally enable raw support for this algorithm and add it to
> > uncalibrated.yaml, so that black levels get reported when capturing
> > tuning images. This is a bit of a hack, because no actual black level
> > correction is taking place in raw mode, but it is the easiest way to get
> > blacklevel reported for raw streams.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/blc.cpp     | 28 +++++++++++++++++++++++++++
> >  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
> >  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
> >  3 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > index 87025e4f8c72..71c62b009707 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,13 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
> >  BlackLevelCorrection::BlackLevelCorrection()
> >         : tuningParameters_(false)
> >  {
> > +       /*
> > +        * This is a bit of a hack. In raw mode no black level correction
> > +        * happens. This flag is used to ensure the metadata gets populated with
> > +        * the black level which is needed to capture proper raw images for
> > +        * tuning.
> > +        */
> > +       supportsRaw_ = true;
> >  }
> >  
> >  /**
> > @@ -107,6 +116,9 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> >                                    [[maybe_unused]] IPAFrameContext &frameContext,
> >                                    rkisp1_params_cfg *params)
> >  {
> > +       if (context.configuration.raw)
> > +               return;
> > +
> >         if (frame > 0)
> >                 return;
> >  
> > @@ -125,6 +137,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)
> > +{
> 
> Hrm. ... now ... should we report SensorBlackLevels at a bit-depth that
> matches the bit-depth of the image?
> 
> I.e ... do we need to scale these 16 bit numbers to 8,10,12 bit
> depending on the sensor configuration and output file bit-depth?

No, that is specified in the control description: ... These values are
returned as numbers out of a 16-bit pixel range (as if pixels ranged
from 0 to 65535)...

That's the reason why I believe it makes sense to unify that all over
libcamera. And I believe it is sensible as otherwise you would have to
explain that the sensor bitwidth and the output bitwidth can be
different and to wich one the black level relates...

Regards,
Stefan

> 
> > +       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:
> >  ...
> > -- 
> > 2.43.0
> >
Kieran Bingham July 3, 2024, 12:30 p.m. UTC | #4 
Quoting Stefan Klug (2024-07-03 13:22:49)
> Hi Kieran,
> 
> On Wed, Jul 03, 2024 at 01:15:55PM +0100, Kieran Bingham wrote:
> > Quoting Stefan Klug (2024-07-03 11:39:51)
> > > Add sensor black levels to the metadata of the rkisp1 pipeline.
> > > 
> > > Additionally enable raw support for this algorithm and add it to
> > > uncalibrated.yaml, so that black levels get reported when capturing
> > > tuning images. This is a bit of a hack, because no actual black level
> > > correction is taking place in raw mode, but it is the easiest way to get
> > > blacklevel reported for raw streams.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/blc.cpp     | 28 +++++++++++++++++++++++++++
> > >  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
> > >  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
> > >  3 files changed, 33 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > > index 87025e4f8c72..71c62b009707 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,13 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
> > >  BlackLevelCorrection::BlackLevelCorrection()
> > >         : tuningParameters_(false)
> > >  {
> > > +       /*
> > > +        * This is a bit of a hack. In raw mode no black level correction
> > > +        * happens. This flag is used to ensure the metadata gets populated with
> > > +        * the black level which is needed to capture proper raw images for
> > > +        * tuning.
> > > +        */
> > > +       supportsRaw_ = true;
> > >  }
> > >  
> > >  /**
> > > @@ -107,6 +116,9 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> > >                                    [[maybe_unused]] IPAFrameContext &frameContext,
> > >                                    rkisp1_params_cfg *params)
> > >  {
> > > +       if (context.configuration.raw)
> > > +               return;
> > > +
> > >         if (frame > 0)
> > >                 return;
> > >  
> > > @@ -125,6 +137,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)
> > > +{
> > 
> > Hrm. ... now ... should we report SensorBlackLevels at a bit-depth that
> > matches the bit-depth of the image?
> > 
> > I.e ... do we need to scale these 16 bit numbers to 8,10,12 bit
> > depending on the sensor configuration and output file bit-depth?
> 
> No, that is specified in the control description: ... These values are
> returned as numbers out of a 16-bit pixel range (as if pixels ranged
> from 0 to 65535)...
> 
> That's the reason why I believe it makes sense to unify that all over
> libcamera. And I believe it is sensible as otherwise you would have to
> explain that the sensor bitwidth and the output bitwidth can be
> different and to wich one the black level relates...

Ack, in that case:

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

I presume it would then be up to the application writing a dng for
instance to know the correct way to write the black level. I "assume"
any black level in a dng file is expected to match the sensor bit-depth,
but I'm absolutely fine with the metadata being stipulated by us, as
that's libcamera metadata and matching the value that would be set on a
control certainly makes sense.



> 
> Regards,
> Stefan
> 
> > 
> > > +       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:
> > >  ...
> > > -- 
> > > 2.43.0
> > >
Laurent Pinchart July 3, 2024, 12:44 p.m. UTC | #5 
On Wed, Jul 03, 2024 at 01:30:58PM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-07-03 13:22:49)
> > On Wed, Jul 03, 2024 at 01:15:55PM +0100, Kieran Bingham wrote:
> > > Quoting Stefan Klug (2024-07-03 11:39:51)
> > > > Add sensor black levels to the metadata of the rkisp1 pipeline.
> > > > 
> > > > Additionally enable raw support for this algorithm and add it to
> > > > uncalibrated.yaml, so that black levels get reported when capturing
> > > > tuning images. This is a bit of a hack, because no actual black level
> > > > correction is taking place in raw mode, but it is the easiest way to get
> > > > blacklevel reported for raw streams.
> > > > 
> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > ---
> > > >  src/ipa/rkisp1/algorithms/blc.cpp     | 28 +++++++++++++++++++++++++++
> > > >  src/ipa/rkisp1/algorithms/blc.h       |  5 ++++-
> > > >  src/ipa/rkisp1/data/uncalibrated.yaml |  1 +
> > > >  3 files changed, 33 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > > > index 87025e4f8c72..71c62b009707 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,13 @@ LOG_DEFINE_CATEGORY(RkISP1Blc)
> > > >  BlackLevelCorrection::BlackLevelCorrection()
> > > >         : tuningParameters_(false)
> > > >  {
> > > > +       /*
> > > > +        * This is a bit of a hack. In raw mode no black level correction
> > > > +        * happens. This flag is used to ensure the metadata gets populated with
> > > > +        * the black level which is needed to capture proper raw images for
> > > > +        * tuning.
> > > > +        */
> > > > +       supportsRaw_ = true;
> > > >  }
> > > >  
> > > >  /**
> > > > @@ -107,6 +116,9 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
> > > >                                    [[maybe_unused]] IPAFrameContext &frameContext,
> > > >                                    rkisp1_params_cfg *params)
> > > >  {
> > > > +       if (context.configuration.raw)
> > > > +               return;
> > > > +
> > > >         if (frame > 0)
> > > >                 return;
> > > >  
> > > > @@ -125,6 +137,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)

Drop [[maybe_unused]] for metadata.

> > > > +{
> > > 
> > > Hrm. ... now ... should we report SensorBlackLevels at a bit-depth that
> > > matches the bit-depth of the image?
> > > 
> > > I.e ... do we need to scale these 16 bit numbers to 8,10,12 bit
> > > depending on the sensor configuration and output file bit-depth?
> > 
> > No, that is specified in the control description: ... These values are
> > returned as numbers out of a 16-bit pixel range (as if pixels ranged
> > from 0 to 65535)...
> > 
> > That's the reason why I believe it makes sense to unify that all over
> > libcamera. And I believe it is sensible as otherwise you would have to
> > explain that the sensor bitwidth and the output bitwidth can be
> > different and to wich one the black level relates...
> 
> Ack, in that case:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> I presume it would then be up to the application writing a dng for
> instance to know the correct way to write the black level. I "assume"
> any black level in a dng file is expected to match the sensor bit-depth,
> but I'm absolutely fine with the metadata being stipulated by us, as
> that's libcamera metadata and matching the value that would be set on a
> control certainly makes sense.

See dng_writer.cpp:

	/* Map the 16-bit value to the bits per sample range. */
	blackLevel[i] = level >> (16 - info->bitsPerSample);

:-)

> > > > +       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:
> > > >  ...

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
index 87025e4f8c72..71c62b009707 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,13 @@  LOG_DEFINE_CATEGORY(RkISP1Blc)
 BlackLevelCorrection::BlackLevelCorrection()
 	: tuningParameters_(false)
 {
+	/*
+	 * This is a bit of a hack. In raw mode no black level correction
+	 * happens. This flag is used to ensure the metadata gets populated with
+	 * the black level which is needed to capture proper raw images for
+	 * tuning.
+	 */
+	supportsRaw_ = true;
 }
 
 /**
@@ -107,6 +116,9 @@  void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context,
 				   [[maybe_unused]] IPAFrameContext &frameContext,
 				   rkisp1_params_cfg *params)
 {
+	if (context.configuration.raw)
+		return;
+
 	if (frame > 0)
 		return;
 
@@ -125,6 +137,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:
 ...