[libcamera-devel,2/3] libcamera: raspberrypi: add sharpness strength control to Raspberry Pi IPAs
diff mbox series

Message ID 20200619092725.19109-3-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • libcamera sharpness strength control
Related show

Commit Message

David Plowman June 19, 2020, 9:27 a.m. UTC 
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    | 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

Comments

Laurent Pinchart June 22, 2020, 2:33 a.m. UTC | #1 
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@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
David Plowman June 22, 2020, 10:33 a.m. UTC | #2 
Hi Laurent

Thanks for the comments!

On Mon, 22 Jun 2020 at 03:33, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@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 :-)

Sure, I'll re-word that a bit better!

>
> > +     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 ?

TBH, probably the right thing to do is to make mode_factor_ a
regular double. SwitchMode can only run synchronously with the
Prepare method, but that might not have been clear to me when
I first wrote this. SetStrength really does run asynchronously, which
is why the std::atomic makes sense. I need to fix the potential
divide-by-zero from your other comments, too.

>
> > +     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 ?

It's a good question. When an image is downscaled the high-pass filter
response, on a per-pixel basis, gets larger (because sharp edges are
in reality always smeared over a few pixels). This leads to
over-sharpening, at least in the hardware that we have. I think you
can argue that downscaling an image by 2x2 causes lengths to halve and
therefore corresponds in some respects to a doubling of some of these
filter responses. It so happens that the "mode factor" that we encode
for our camera modes represents the same thing. But I'd be lying if I
didn't confess it's also rather empirical!

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

No problem!

Version 2 of all this incoming shortly...

Best regards
David

>
> > +     // 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

Patch
diff mbox series 

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.
+	double user_strength = user_strength_;
+	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;
+	status.threshold = threshold_ * mode_factor / user_strength_sqrt;
+	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 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