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

[Laurent Pinchart]

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 at 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

-- 
Regards,

Laurent Pinchart