[libcamera-devel] [PATCH v3 3/4] libcamera: ipa: raspberrypi: Add sharpness strength control

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Tue, Jun 23, 2020 at 10:14:03AM +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    | 28 +++++++++++++++----
>  .../raspberrypi/controller/rpi/sharpen.hpp    |  6 ++--
>  .../controller/sharpen_algorithm.hpp          | 21 ++++++++++++++
>  .../raspberrypi/controller/sharpen_status.h   |  2 ++
>  4 files changed, 50 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..3bae01f 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,32 @@ 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. Negative
> +	// values are not allowed - coerce them to zero (no sharpening).
> +	user_strength_ = std::max(0.0, 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_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);

checkstyle.py reports that std::max should be aligned under threshold_.
If you don't use it already, I recommend setting up checkstyle.py as a
git hook. You can simply copy utils/hooks/post-commit or
utils/hooks/pre-commit to .git/hooks/. I personally use the post-commit
hook to make it easier to ignore false positives.

With this small issue fixed,

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

> +	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..568521b 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_;
> +	double user_strength_;
>  };
>  
>  } // 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