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

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Fri, Jun 19, 2020 at 10:27:24AM +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    | 23 +++++++++++++++----
>  .../raspberrypi/controller/rpi/sharpen.hpp    |  6 +++--
>  .../controller/sharpen_algorithm.hpp          | 21 +++++++++++++++++
>  .../raspberrypi/controller/sharpen_status.h   |  2 ++
>  4 files changed, 46 insertions(+), 6 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 1f07bb6..45fe3fe 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)
>  {
>  }
>  
> @@ -40,15 +40,30 @@ 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)
>  {
>  	double mode_factor = mode_factor_;
> +	// The user_strength affects the algorithm's internal gain directly, but
> +	// we adjust the limit and threshold more cautiously, hence the sqrt.

I didn't understand this initially, but reading your camera tuning guide
helped. I think s/we adjust/we want to adjust/ would be a bit clearer.
Maybe "hence the sqrt" could also be reworded to clarify that it's an
arbitrary heuristic. Or, if it isn't, then my understanding is incorrect
and some more information would be useful :-)

> +	double user_strength = user_strength_;

Do you need this local variable, can't you use user_strength_ directly
in the code below ? Or are there multi-threading race conditions ? If
so, doesn't the same race condition affect mode_factor and user_strength
as you read them both here without any lock ?

> +	double user_strength_sqrt = sqrt(user_strength);
>  	struct SharpenStatus status;
>  	// Binned modes seem to need the sharpening toned down with this
>  	// pipeline.

Out of curiosity, why is that ?

> -	status.threshold = threshold_ * mode_factor;
> -	status.strength = strength_ / mode_factor;
> -	status.limit = limit_ / mode_factor;
> +	status.threshold = threshold_ * mode_factor / user_strength_sqrt;
> +	status.strength = (strength_ / mode_factor) * user_strength;
> +	status.limit = (limit_ / mode_factor) * user_strength_sqrt;

The parentheses are not needed in the above two lines.

> +	// 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 3b0d680..5b419b4 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) 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_;
>  	std::atomic<double> mode_factor_;
> +	std::atomic<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