[libcamera-devel,v2,3/4] libcamera: ipa: raspberrypi: add sharpness strength control to Raspberry Pi IPAs

4111 diff mbox series

The sharpness control is, loosely speaking, a gain applied to
the amount of sharpening added to an image. We also report the
sharpness setting used back to the caller in metadata.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 .../raspberrypi/controller/rpi/sharpen.cpp    | 30 +++++++++++++++----
 .../raspberrypi/controller/rpi/sharpen.hpp    |  6 ++--
 .../controller/sharpen_algorithm.hpp          | 21 +++++++++++++
 .../raspberrypi/controller/sharpen_status.h   |  2 ++
 4 files changed, 52 insertions(+), 7 deletions(-)
 create mode 100644 src/ipa/raspberrypi/controller/sharpen_algorithm.hpp

Hi David,

Thank you for the patch.

To try and keep the subject line short, how about

"libcamera: ipa: raspberrypi: Add sharpness strength control"

as the prefix implies it's for the Raspberry Pi IPA ? By the way, I
don't think there's any written rule, but we tend to capitalize the
sentence in the subject line after the prefix.

On Mon, Jun 22, 2020 at 02:55:49PM +0100, David Plowman wrote:
> The sharpness control is, loosely speaking, a gain applied to
> the amount of sharpening added to an image. We also report the
> sharpness setting used back to the caller in metadata.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  .../raspberrypi/controller/rpi/sharpen.cpp    | 30 +++++++++++++++----
>  .../raspberrypi/controller/rpi/sharpen.hpp    |  6 ++--
>  .../controller/sharpen_algorithm.hpp          | 21 +++++++++++++
>  .../raspberrypi/controller/sharpen_status.h   |  2 ++
>  4 files changed, 52 insertions(+), 7 deletions(-)
>  create mode 100644 src/ipa/raspberrypi/controller/sharpen_algorithm.hpp
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp
> index 4c2fdb3..cd93529 100644
> --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp
> @@ -17,7 +17,7 @@ using namespace RPi;
>  #define NAME "rpi.sharpen"
>  
>  Sharpen::Sharpen(Controller *controller)
> -	: Algorithm(controller)
> +	: SharpenAlgorithm(controller), user_strength_(1.0)
>  {
>  }
>  
> @@ -42,14 +42,34 @@ void Sharpen::Read(boost::property_tree::ptree const &params)
>  	limit_ = params.get<double>("limit", 1.0);
>  }
>  
> +void Sharpen::SetStrength(double strength)
> +{
> +	// Note that this method is how an application sets the overall
> +	// sharpening "strength". We call this the "user strength" field
> +	// as there already is a strength_ field - being an internal gain
> +	// parameter that gets passed to the ISP control code.
> +	user_strength_ = strength;
> +}
> +
>  void Sharpen::Prepare(Metadata *image_metadata)
>  {
> +	// The user_strength affects the algorithm's internal gain directly, but
> +	// we adjust the limit and threshold less aggressively. Using a sqrt
> +	// function is an arbitrary but gentle way of accomplishing this.
> +	double user_strength = user_strength_;
> +	if (user_strength < 0.0)
> +		user_strength = 0.0;

I was going to question if this was required, as the minimum is 0.0,
only to realize that the libcamera core doesn't enforce constraints on
controls. This is something we need to fix in the core, should we drop
the check here ? You could then drop the local variable.

> +	double user_strength_sqrt = sqrt(user_strength);
>  	struct SharpenStatus status;
>  	// Binned modes seem to need the sharpening toned down with this
> -	// pipeline.
> -	status.threshold = threshold_ * mode_factor_;
> -	status.strength = strength_ / mode_factor_;
> -	status.limit = limit_ / mode_factor_;
> +	// pipeline, thus we use the mode_factor here. Also avoid
> +	// divide-by-zero with the user_strength_sqrt.
> +	status.threshold = threshold_ * mode_factor_ /
> +		std::max(0.01, user_strength_sqrt);

If the user strength is very small this would be a large number. I
wonder if it could overflow the range supported by the hardware, but as
the limit would be very low, I suppose it would likely not matter. Could
you confirm the hardware won't get confused ?

> +	status.strength = strength_ / mode_factor_ * user_strength;
> +	status.limit = limit_ / mode_factor_ * user_strength_sqrt;
> +	// Finally, report any application-supplied parameters that were used.
> +	status.user_strength = user_strength;
>  	image_metadata->Set("sharpen.status", status);
>  }
>  
> diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp
> index a3bf899..e9a71fd 100644
> --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp
> @@ -6,20 +6,21 @@
>   */
>  #pragma once
>  
> -#include "../algorithm.hpp"
> +#include "../sharpen_algorithm.hpp"
>  #include "../sharpen_status.h"
>  
>  // This is our implementation of the "sharpen algorithm".
>  
>  namespace RPi {
>  
> -class Sharpen : public Algorithm
> +class Sharpen : public SharpenAlgorithm
>  {
>  public:
>  	Sharpen(Controller *controller);
>  	char const *Name() const override;
>  	void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;
>  	void Read(boost::property_tree::ptree const &params) override;
> +	void SetStrength(double strength) override;
>  	void Prepare(Metadata *image_metadata) override;
>  
>  private:
> @@ -27,6 +28,7 @@ private:
>  	double strength_;
>  	double limit_;
>  	double mode_factor_;
> +	std::atomic<double> user_strength_;

This this need to be an atomic variable, as it's only accessed from
Prepare and SetStrengh, both called from IPARPi::processEvent() only ?

>  };
>  
>  } // namespace RPi
> diff --git a/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp b/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp
> new file mode 100644
> index 0000000..3b27a74
> --- /dev/null
> +++ b/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited
> + *
> + * sharpen_algorithm.hpp - sharpness control algorithm interface
> + */
> +#pragma once
> +
> +#include "algorithm.hpp"
> +
> +namespace RPi {
> +
> +class SharpenAlgorithm : public Algorithm
> +{
> +public:
> +	SharpenAlgorithm(Controller *controller) : Algorithm(controller) {}
> +	// A sharpness control algorithm must provide the following:
> +	virtual void SetStrength(double strength) = 0;
> +};
> +
> +} // namespace RPi
> diff --git a/src/ipa/raspberrypi/controller/sharpen_status.h b/src/ipa/raspberrypi/controller/sharpen_status.h
> index 6de80f4..7501b19 100644
> --- a/src/ipa/raspberrypi/controller/sharpen_status.h
> +++ b/src/ipa/raspberrypi/controller/sharpen_status.h
> @@ -19,6 +19,8 @@ struct SharpenStatus {
>  	double strength;
>  	// upper limit of the allowed sharpening response
>  	double limit;
> +	// The sharpening strength requested by the user or application.
> +	double user_strength;
>  };
>  
>  #ifdef __cplusplus

Hi Laurent

Thanks again for the comments.

On Tue, 23 Jun 2020 at 00:05, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> To try and keep the subject line short, how about
>
> "libcamera: ipa: raspberrypi: Add sharpness strength control"
>
> as the prefix implies it's for the Raspberry Pi IPA ? By the way, I
> don't think there's any written rule, but we tend to capitalize the
> sentence in the subject line after the prefix.

Sure, no need to repeat Raspberry Pi and all that. I'll fix up both
comments in v3!

>
> On Mon, Jun 22, 2020 at 02:55:49PM +0100, David Plowman wrote:
> > The sharpness control is, loosely speaking, a gain applied to
> > the amount of sharpening added to an image. We also report the
> > sharpness setting used back to the caller in metadata.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  .../raspberrypi/controller/rpi/sharpen.cpp    | 30 +++++++++++++++----
> >  .../raspberrypi/controller/rpi/sharpen.hpp    |  6 ++--
> >  .../controller/sharpen_algorithm.hpp          | 21 +++++++++++++
> >  .../raspberrypi/controller/sharpen_status.h   |  2 ++
> >  4 files changed, 52 insertions(+), 7 deletions(-)
> >  create mode 100644 src/ipa/raspberrypi/controller/sharpen_algorithm.hpp
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp
> > index 4c2fdb3..cd93529 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp
> > @@ -17,7 +17,7 @@ using namespace RPi;
> >  #define NAME "rpi.sharpen"
> >
> >  Sharpen::Sharpen(Controller *controller)
> > -     : Algorithm(controller)
> > +     : SharpenAlgorithm(controller), user_strength_(1.0)
> >  {
> >  }
> >
> > @@ -42,14 +42,34 @@ void Sharpen::Read(boost::property_tree::ptree const &params)
> >       limit_ = params.get<double>("limit", 1.0);
> >  }
> >
> > +void Sharpen::SetStrength(double strength)
> > +{
> > +     // Note that this method is how an application sets the overall
> > +     // sharpening "strength". We call this the "user strength" field
> > +     // as there already is a strength_ field - being an internal gain
> > +     // parameter that gets passed to the ISP control code.
> > +     user_strength_ = strength;
> > +}
> > +
> >  void Sharpen::Prepare(Metadata *image_metadata)
> >  {
> > +     // The user_strength affects the algorithm's internal gain directly, but
> > +     // we adjust the limit and threshold less aggressively. Using a sqrt
> > +     // function is an arbitrary but gentle way of accomplishing this.
> > +     double user_strength = user_strength_;
> > +     if (user_strength < 0.0)
> > +             user_strength = 0.0;
>
> I was going to question if this was required, as the minimum is 0.0,
> only to realize that the libcamera core doesn't enforce constraints on
> controls. This is something we need to fix in the core, should we drop
> the check here ? You could then drop the local variable.

I don't feel terribly strongly - maybe if we make that change in the
core then we can fix up this sort of thing when we next revisit the
affected files? Until then I'm inclined to leave it unless anyone
objects...

>
> > +     double user_strength_sqrt = sqrt(user_strength);
> >       struct SharpenStatus status;
> >       // Binned modes seem to need the sharpening toned down with this
> > -     // pipeline.
> > -     status.threshold = threshold_ * mode_factor_;
> > -     status.strength = strength_ / mode_factor_;
> > -     status.limit = limit_ / mode_factor_;
> > +     // pipeline, thus we use the mode_factor here. Also avoid
> > +     // divide-by-zero with the user_strength_sqrt.
> > +     status.threshold = threshold_ * mode_factor_ /
> > +             std::max(0.01, user_strength_sqrt);
>
> If the user strength is very small this would be a large number. I
> wonder if it could overflow the range supported by the hardware, but as
> the limit would be very low, I suppose it would likely not matter. Could
> you confirm the hardware won't get confused ?

Yes, it actually travels through to the GPU with quite a lot of bits,
and then gets clamped over there before values are programmed
into the ISP registers.

>
> > +     status.strength = strength_ / mode_factor_ * user_strength;
> > +     status.limit = limit_ / mode_factor_ * user_strength_sqrt;
> > +     // Finally, report any application-supplied parameters that were used.
> > +     status.user_strength = user_strength;
> >       image_metadata->Set("sharpen.status", status);
> >  }
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp
> > index a3bf899..e9a71fd 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp
> > @@ -6,20 +6,21 @@
> >   */
> >  #pragma once
> >
> > -#include "../algorithm.hpp"
> > +#include "../sharpen_algorithm.hpp"
> >  #include "../sharpen_status.h"
> >
> >  // This is our implementation of the "sharpen algorithm".
> >
> >  namespace RPi {
> >
> > -class Sharpen : public Algorithm
> > +class Sharpen : public SharpenAlgorithm
> >  {
> >  public:
> >       Sharpen(Controller *controller);
> >       char const *Name() const override;
> >       void SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;
> >       void Read(boost::property_tree::ptree const &params) override;
> > +     void SetStrength(double strength) override;
> >       void Prepare(Metadata *image_metadata) override;
> >
> >  private:
> > @@ -27,6 +28,7 @@ private:
> >       double strength_;
> >       double limit_;
> >       double mode_factor_;
> > +     std::atomic<double> user_strength_;
>
> This this need to be an atomic variable, as it's only accessed from
> Prepare and SetStrengh, both called from IPARPi::processEvent() only ?

Well, I suppose that's true. Of course all this code comes from our
pre-libcamera days (was there such a time??) when the API model was
that applications call our control algorithms directly (and
asynchronously), so there's probably quite a bit of this in
practically every "control setting" function that we have.

Anyway, I propose to fix this one up now (in v3 of this patch), and
will leave the others until such time as we have a need to touch the
files in question, if folks are OK with that.

Thanks again
David

>
> >  };
> >
> >  } // namespace RPi
> > diff --git a/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp b/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp
> > new file mode 100644
> > index 0000000..3b27a74
> > --- /dev/null
> > +++ b/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited
> > + *
> > + * sharpen_algorithm.hpp - sharpness control algorithm interface
> > + */
> > +#pragma once
> > +
> > +#include "algorithm.hpp"
> > +
> > +namespace RPi {
> > +
> > +class SharpenAlgorithm : public Algorithm
> > +{
> > +public:
> > +     SharpenAlgorithm(Controller *controller) : Algorithm(controller) {}
> > +     // A sharpness control algorithm must provide the following:
> > +     virtual void SetStrength(double strength) = 0;
> > +};
> > +
> > +} // namespace RPi
> > diff --git a/src/ipa/raspberrypi/controller/sharpen_status.h b/src/ipa/raspberrypi/controller/sharpen_status.h
> > index 6de80f4..7501b19 100644
> > --- a/src/ipa/raspberrypi/controller/sharpen_status.h
> > +++ b/src/ipa/raspberrypi/controller/sharpen_status.h
> > @@ -19,6 +19,8 @@ struct SharpenStatus {
> >       double strength;
> >       // upper limit of the allowed sharpening response
> >       double limit;
> > +     // The sharpening strength requested by the user or application.
> > +     double user_strength;
> >  };
> >
> >  #ifdef __cplusplus
>
> --
> Regards,
>
> Laurent Pinchart