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

[David Plowman]

Hi Laurent

Thanks for the comments!

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

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