| Message ID | 20200701125021.1388-1-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David, 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@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' > +LOG_DEFINE_CATEGORY(RPI_FOCUS) > > #define NAME "rpi.focus" > > @@ -24,11 +28,6 @@ char const *Focus::Name() const > return NAME; > } > > -void Focus::Read(boost::property_tree::ptree const ¶ms) > -{ > - 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 > } > > /* 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 ¶ms) 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 ? Minors apart, which can be fixed while applying: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > } > } > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo Thanks for the review! On Wed, 1 Jul 2020 at 17:09, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, > > 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@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' No problem with that! > > > +LOG_DEFINE_CATEGORY(RPI_FOCUS) > > > > #define NAME "rpi.focus" > > > > @@ -24,11 +28,6 @@ char const *Focus::Name() const > > return NAME; > > } > > > > -void Focus::Read(boost::property_tree::ptree const ¶ms) > > -{ > > - 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... > > > } > > > > /* 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 ¶ms) 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... Best regards David > > Minors apart, which can be fixed while applying: > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > > > } > > } > > -- > > 2.20.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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 ¶ms) > > > -{ > > > - 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 ¶ms) 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@ideasonboard.com> > > Minors apart, which can be fixed while applying: > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > } > > > }
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@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@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@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 ¶ms) > > > > -{ > > > > - 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 ¶ms) 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@ideasonboard.com> > > > > Minors apart, which can be fixed while applying: > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > } > > > > } > > -- > Regards, > > Laurent Pinchart
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; + +LOG_DEFINE_CATEGORY(RPI_FOCUS) #define NAME "rpi.focus" @@ -24,11 +28,6 @@ char const *Focus::Name() const return NAME; } -void Focus::Read(boost::property_tree::ptree const ¶ms) -{ - 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; } /* 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 ¶ms) 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 + } }
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@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(-)