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

[Laurent Pinchart]

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