[libcamera-devel] [PATCH v2] libcamera: ipa: raspberrypi: Enable focus measure without recompile

[David Plowman]

Hi Laurent

Thanks for the various changes, they're all fine with me!

And I'll review the ctt patches today. Thanks for all that work, I
never thought anyone was going to touch that ever again!

Best regards
David

On Fri, 3 Jul 2020 at 01:17, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David and Jacopo,
>
> On Wed, Jul 01, 2020 at 05:25:00PM +0100, David Plowman wrote:
> > On Wed, 1 Jul 2020 at 17:09, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > On Wed, Jul 01, 2020 at 01:50:21PM +0100, David Plowman wrote:
> > > > Previously, output of the focus measure could not be enabled without
> > > > recompiling (because of the RPI_LOGGING_ENABLE macro). This uses the
> > > > libcamera logging mechanism instead, so can be enabled/disabled at
> > > > runtime.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > >  src/ipa/raspberrypi/controller/rpi/focus.cpp | 16 ++++++----------
> > > >  src/ipa/raspberrypi/controller/rpi/focus.hpp |  3 ---
> > > >  src/ipa/raspberrypi/data/imx477.json         |  2 +-
> > > >  3 files changed, 7 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp
> > > > index 1e2b649..573831b 100644
> > > > --- a/src/ipa/raspberrypi/controller/rpi/focus.cpp
> > > > +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp
> > > > @@ -6,11 +6,15 @@
> > > >   */
> > > >  #include <stdint.h>
> > > >
> > > > +#include "libcamera/internal/log.h"
> > > > +
> > > >  #include "../focus_status.h"
> > > > -#include "../logging.hpp"
> > > >  #include "focus.hpp"
> > > >
> > > >  using namespace RPi;
> > > > +using namespace libcamera;
> > > > +
> > >
> > > OCD says 'l' comes before 'R'
>
> >>> using = ['RPi', 'libcamera']
> >>> using.sort()
> >>> using
> ['RPi', 'libcamera']
>
> Python disagrees with your OCD ;-)
>
> > No problem with that!
> >
> > > > +LOG_DEFINE_CATEGORY(RPI_FOCUS)
>
> We tend to use CamelCase for log categories. Would RPiFocus be OK for
> you ?
>
> I may work in supporting subcategories at some point, in which case this
> would become RPi.Focus (the name used with the LOG macro would will
> still be RPiFocus though, that one needs to be a valid C++ symbol name).
>
> > > >
> > > >  #define NAME "rpi.focus"
> > > >
> > > > @@ -24,11 +28,6 @@ char const *Focus::Name() const
> > > >       return NAME;
> > > >  }
> > > >
> > > > -void Focus::Read(boost::property_tree::ptree const &params)
> > > > -{
> > > > -     print_ = params.get<int>("print", 0);
> > > > -}
> > > > -
> > > >  void Focus::Process(StatisticsPtr &stats, Metadata *image_metadata)
> > > >  {
> > > >       FocusStatus status;
> > > > @@ -37,10 +36,7 @@ void Focus::Process(StatisticsPtr &stats, Metadata *image_metadata)
> > > >               status.focus_measures[i] = stats->focus_stats[i].contrast_val[1][1] / 1000;
> > > >       status.num = i;
> > > >       image_metadata->Set("focus.status", status);
> > > > -     if (print_) {
> > > > -             uint32_t value = (status.focus_measures[5] + status.focus_measures[6]) / 10;
> > > > -             RPI_LOG("Focus contrast measure: " << value);
> > > > -     }
> > > > +     LOG(RPI_FOCUS, Debug) << "Focus contrast measure: " << (status.focus_measures[5] + status.focus_measures[6]) / 10;
> > >
> > > maybe break this line
> >
> > Pretty sure I had it like that first but checkstyle complained, and
> > then it was OK with this. But happy if we prefer to change it...
>
> libcamera has a hard 120 columns limit, and a soft 80 columns limit.
> This means we prefer keeping lines under 80 columns, but going over up
> to 120 is fine when it improves readability. checkstyle.py relies on
> clang-format, which doesn't support soft and hard limits, so it will
> sometimes propose removing line breaks.
>
> And now that I've written this, I see that our .clang-format file has
> ColumnLimit set to 0 (no limit). I have thus no idea what's going on :-)
>
> > > >  }
> > > >
> > > >  /* Register algorithm with the system. */
> > > > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.hpp b/src/ipa/raspberrypi/controller/rpi/focus.hpp
> > > > index d53401f..a9756ea 100644
> > > > --- a/src/ipa/raspberrypi/controller/rpi/focus.hpp
> > > > +++ b/src/ipa/raspberrypi/controller/rpi/focus.hpp
> > > > @@ -22,10 +22,7 @@ class Focus : public Algorithm
> > > >  public:
> > > >       Focus(Controller *controller);
> > > >       char const *Name() const override;
> > > > -     void Read(boost::property_tree::ptree const &params) override;
> > > >       void Process(StatisticsPtr &stats, Metadata *image_metadata) override;
> > > > -private:
> > > > -     bool print_;
> > > >  };
> > > >
> > > >  } /* namespace RPi */
> > > > diff --git a/src/ipa/raspberrypi/data/imx477.json b/src/ipa/raspberrypi/data/imx477.json
> > > > index 389e8ce..747cd8d 100644
> > > > --- a/src/ipa/raspberrypi/data/imx477.json
> > > > +++ b/src/ipa/raspberrypi/data/imx477.json
> > > > @@ -415,6 +415,6 @@
> > > >      },
> > > >      "rpi.focus":
> > > >      {
> > > > -     "print": 1
> > > > +
> > >
> > > Is the empty line intentional ?
> >
> > Well, for reasons I'm not totally clear about, our Python tuning tool
> > tends to produce a blank line when there's nothing between the curlies
> > - so I just tend to copy it like that. Mostly. But not always. So I'm
> > fine to delete it too...
>
> "[PATCH 00/10] utils: raspberrypi: ctt: Improve JSON pretty printer"
>
> I'll delete the empty line :-)
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > > Minors apart, which can be fixed while applying:
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > >
> > > >      }
> > > >  }
>
> --
> Regards,
>
> Laurent Pinchart