| Message ID | 20200528145132.96102-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. On Thu, May 28, 2020 at 03:51:32PM +0100, Naushir Patuck wrote: > From: David Plowman <david.plowman@raspberrypi.com> > > Adds FocusStatus to the image metadata, containing contrast measurements > across the image. Optionally also prints a contrast measure to the > console, to aid in manual adjustment of the lens. Note that it is not an > actual auto-focus algorithm that can drive a lens! An interesting exercise for a motivated reader :-) > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/focus_status.h | 26 ++++++++++ > src/ipa/raspberrypi/controller/rpi/focus.cpp | 51 +++++++++++++++++++ > src/ipa/raspberrypi/controller/rpi/focus.hpp | 31 +++++++++++ > src/ipa/raspberrypi/data/imx477.json | 4 ++ > src/ipa/raspberrypi/meson.build | 1 + > 5 files changed, 113 insertions(+) > create mode 100644 src/ipa/raspberrypi/controller/focus_status.h > create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.cpp > create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.hpp > > diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h > new file mode 100644 > index 00000000..3ad88777 > --- /dev/null > +++ b/src/ipa/raspberrypi/controller/focus_status.h > @@ -0,0 +1,26 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > + * > + * focus_status.h - focus measurement status > + */ > +#pragma once > + > +#include <linux/bcm2835-isp.h> > + > +// The focus algorithm should post the following structure into the image's > +// "focus.status" metadata. Recall that it's only reporting focus (contrast) > +// measurements, it's not driving any kind of auto-focus algorithm! > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +struct FocusStatus { > + int num; > + uint32_t focus_measures[FOCUS_REGIONS]; > +}; > + > +#ifdef __cplusplus > +} > +#endif > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp > new file mode 100644 > index 00000000..d6cdc4bf > --- /dev/null > +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp > @@ -0,0 +1,51 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > + * > + * focus.cpp - focus algorithm > + */ > +#include <stdint.h> > + > +#include "../focus_status.h" > +#include "../logging.hpp" > +#include "focus.hpp" > + > +using namespace RPi; > + > +#define NAME "rpi.focus" > + > +Focus::Focus(Controller *controller) > + : Algorithm(controller) > +{ > +} > + > +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; > + int i; As i is never negative, should it be an unsigned int ? Apart from this, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Let me know if you're fine with the change, in which case I'll modify the patch when applying. > + for (i = 0; i < FOCUS_REGIONS; i++) > + 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); > + } > +} > + > +/* Register algorithm with the system. */ > +static Algorithm *Create(Controller *controller) > +{ > + return new Focus(controller); > +} > +static RegisterAlgorithm reg(NAME, &Create); > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.hpp b/src/ipa/raspberrypi/controller/rpi/focus.hpp > new file mode 100644 > index 00000000..d53401f7 > --- /dev/null > +++ b/src/ipa/raspberrypi/controller/rpi/focus.hpp > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > + * > + * focus.hpp - focus algorithm > + */ > +#pragma once > + > +#include "../algorithm.hpp" > +#include "../metadata.hpp" > + > +/* > + * The "focus" algorithm. All it does it print out a version of the > + * focus contrast measure; there is no actual auto-focus mechanism to > + * control. > + */ > + > +namespace RPi { > + > +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 dce5234f..389e8ce8 100644 > --- a/src/ipa/raspberrypi/data/imx477.json > +++ b/src/ipa/raspberrypi/data/imx477.json > @@ -412,5 +412,9 @@ > "rpi.sharpen": > { > > + }, > + "rpi.focus": > + { > + "print": 1 > } > } > diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build > index 697902e9..9445cd09 100644 > --- a/src/ipa/raspberrypi/meson.build > +++ b/src/ipa/raspberrypi/meson.build > @@ -29,6 +29,7 @@ rpi_ipa_sources = files([ > 'controller/rpi/awb.cpp', > 'controller/rpi/sharpen.cpp', > 'controller/rpi/black_level.cpp', > + 'controller/rpi/focus.cpp', > 'controller/rpi/geq.cpp', > 'controller/rpi/noise.cpp', > 'controller/rpi/lux.cpp',
On Thu, Jun 25, 2020 at 06:46:05AM +0300, Laurent Pinchart wrote: > Hi David, I meant Naush. Or Naush and David. Or the other way around. Or I should go to bed :-) > Thank you for the patch. > > On Thu, May 28, 2020 at 03:51:32PM +0100, Naushir Patuck wrote: > > From: David Plowman <david.plowman@raspberrypi.com> > > > > Adds FocusStatus to the image metadata, containing contrast measurements > > across the image. Optionally also prints a contrast measure to the > > console, to aid in manual adjustment of the lens. Note that it is not an > > actual auto-focus algorithm that can drive a lens! > > An interesting exercise for a motivated reader :-) > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/focus_status.h | 26 ++++++++++ > > src/ipa/raspberrypi/controller/rpi/focus.cpp | 51 +++++++++++++++++++ > > src/ipa/raspberrypi/controller/rpi/focus.hpp | 31 +++++++++++ > > src/ipa/raspberrypi/data/imx477.json | 4 ++ > > src/ipa/raspberrypi/meson.build | 1 + > > 5 files changed, 113 insertions(+) > > create mode 100644 src/ipa/raspberrypi/controller/focus_status.h > > create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.cpp > > create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.hpp > > > > diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h > > new file mode 100644 > > index 00000000..3ad88777 > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/focus_status.h > > @@ -0,0 +1,26 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > > + * > > + * focus_status.h - focus measurement status > > + */ > > +#pragma once > > + > > +#include <linux/bcm2835-isp.h> > > + > > +// The focus algorithm should post the following structure into the image's > > +// "focus.status" metadata. Recall that it's only reporting focus (contrast) > > +// measurements, it's not driving any kind of auto-focus algorithm! > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +struct FocusStatus { > > + int num; > > + uint32_t focus_measures[FOCUS_REGIONS]; > > +}; > > + > > +#ifdef __cplusplus > > +} > > +#endif > > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp > > new file mode 100644 > > index 00000000..d6cdc4bf > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp > > @@ -0,0 +1,51 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > > + * > > + * focus.cpp - focus algorithm > > + */ > > +#include <stdint.h> > > + > > +#include "../focus_status.h" > > +#include "../logging.hpp" > > +#include "focus.hpp" > > + > > +using namespace RPi; > > + > > +#define NAME "rpi.focus" > > + > > +Focus::Focus(Controller *controller) > > + : Algorithm(controller) > > +{ > > +} > > + > > +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; > > + int i; > > As i is never negative, should it be an unsigned int ? > > Apart from this, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Let me know if you're fine with the change, in which case I'll modify > the patch when applying. > > > + for (i = 0; i < FOCUS_REGIONS; i++) > > + 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); > > + } > > +} > > + > > +/* Register algorithm with the system. */ > > +static Algorithm *Create(Controller *controller) > > +{ > > + return new Focus(controller); > > +} > > +static RegisterAlgorithm reg(NAME, &Create); > > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.hpp b/src/ipa/raspberrypi/controller/rpi/focus.hpp > > new file mode 100644 > > index 00000000..d53401f7 > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/rpi/focus.hpp > > @@ -0,0 +1,31 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > > + * > > + * focus.hpp - focus algorithm > > + */ > > +#pragma once > > + > > +#include "../algorithm.hpp" > > +#include "../metadata.hpp" > > + > > +/* > > + * The "focus" algorithm. All it does it print out a version of the > > + * focus contrast measure; there is no actual auto-focus mechanism to > > + * control. > > + */ > > + > > +namespace RPi { > > + > > +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 dce5234f..389e8ce8 100644 > > --- a/src/ipa/raspberrypi/data/imx477.json > > +++ b/src/ipa/raspberrypi/data/imx477.json > > @@ -412,5 +412,9 @@ > > "rpi.sharpen": > > { > > > > + }, > > + "rpi.focus": > > + { > > + "print": 1 > > } > > } > > diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build > > index 697902e9..9445cd09 100644 > > --- a/src/ipa/raspberrypi/meson.build > > +++ b/src/ipa/raspberrypi/meson.build > > @@ -29,6 +29,7 @@ rpi_ipa_sources = files([ > > 'controller/rpi/awb.cpp', > > 'controller/rpi/sharpen.cpp', > > 'controller/rpi/black_level.cpp', > > + 'controller/rpi/focus.cpp', > > 'controller/rpi/geq.cpp', > > 'controller/rpi/noise.cpp', > > 'controller/rpi/lux.cpp',
Hi Laurent Thanks for the review. On Thu, 25 Jun 2020 at 04:46, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > Thank you for the patch. > > On Thu, May 28, 2020 at 03:51:32PM +0100, Naushir Patuck wrote: > > From: David Plowman <david.plowman@raspberrypi.com> > > > > Adds FocusStatus to the image metadata, containing contrast measurements > > across the image. Optionally also prints a contrast measure to the > > console, to aid in manual adjustment of the lens. Note that it is not an > > actual auto-focus algorithm that can drive a lens! > > An interesting exercise for a motivated reader :-) > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/focus_status.h | 26 ++++++++++ > > src/ipa/raspberrypi/controller/rpi/focus.cpp | 51 +++++++++++++++++++ > > src/ipa/raspberrypi/controller/rpi/focus.hpp | 31 +++++++++++ > > src/ipa/raspberrypi/data/imx477.json | 4 ++ > > src/ipa/raspberrypi/meson.build | 1 + > > 5 files changed, 113 insertions(+) > > create mode 100644 src/ipa/raspberrypi/controller/focus_status.h > > create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.cpp > > create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.hpp > > > > diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h > > new file mode 100644 > > index 00000000..3ad88777 > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/focus_status.h > > @@ -0,0 +1,26 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > > + * > > + * focus_status.h - focus measurement status > > + */ > > +#pragma once > > + > > +#include <linux/bcm2835-isp.h> > > + > > +// The focus algorithm should post the following structure into the image's > > +// "focus.status" metadata. Recall that it's only reporting focus (contrast) > > +// measurements, it's not driving any kind of auto-focus algorithm! > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +struct FocusStatus { > > + int num; > > + uint32_t focus_measures[FOCUS_REGIONS]; > > +}; > > + > > +#ifdef __cplusplus > > +} > > +#endif > > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp > > new file mode 100644 > > index 00000000..d6cdc4bf > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp > > @@ -0,0 +1,51 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > > + * > > + * focus.cpp - focus algorithm > > + */ > > +#include <stdint.h> > > + > > +#include "../focus_status.h" > > +#include "../logging.hpp" > > +#include "focus.hpp" > > + > > +using namespace RPi; > > + > > +#define NAME "rpi.focus" > > + > > +Focus::Focus(Controller *controller) > > + : Algorithm(controller) > > +{ > > +} > > + > > +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; > > + int i; > > As i is never negative, should it be an unsigned int ? That's fine with me, thanks! Thanks again and best regards David > > Apart from this, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Let me know if you're fine with the change, in which case I'll modify > the patch when applying. > > > + for (i = 0; i < FOCUS_REGIONS; i++) > > + 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); > > + } > > +} > > + > > +/* Register algorithm with the system. */ > > +static Algorithm *Create(Controller *controller) > > +{ > > + return new Focus(controller); > > +} > > +static RegisterAlgorithm reg(NAME, &Create); > > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.hpp b/src/ipa/raspberrypi/controller/rpi/focus.hpp > > new file mode 100644 > > index 00000000..d53401f7 > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/rpi/focus.hpp > > @@ -0,0 +1,31 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > > + * > > + * focus.hpp - focus algorithm > > + */ > > +#pragma once > > + > > +#include "../algorithm.hpp" > > +#include "../metadata.hpp" > > + > > +/* > > + * The "focus" algorithm. All it does it print out a version of the > > + * focus contrast measure; there is no actual auto-focus mechanism to > > + * control. > > + */ > > + > > +namespace RPi { > > + > > +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 dce5234f..389e8ce8 100644 > > --- a/src/ipa/raspberrypi/data/imx477.json > > +++ b/src/ipa/raspberrypi/data/imx477.json > > @@ -412,5 +412,9 @@ > > "rpi.sharpen": > > { > > > > + }, > > + "rpi.focus": > > + { > > + "print": 1 > > } > > } > > diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build > > index 697902e9..9445cd09 100644 > > --- a/src/ipa/raspberrypi/meson.build > > +++ b/src/ipa/raspberrypi/meson.build > > @@ -29,6 +29,7 @@ rpi_ipa_sources = files([ > > 'controller/rpi/awb.cpp', > > 'controller/rpi/sharpen.cpp', > > 'controller/rpi/black_level.cpp', > > + 'controller/rpi/focus.cpp', > > 'controller/rpi/geq.cpp', > > 'controller/rpi/noise.cpp', > > 'controller/rpi/lux.cpp', > > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi David, Naush, On 28/05/2020 15:51, Naushir Patuck wrote: > From: David Plowman <david.plowman@raspberrypi.com> > > Adds FocusStatus to the image metadata, containing contrast measurements > across the image. Optionally also prints a contrast measure to the > console, to aid in manual adjustment of the lens. Note that it is not an > actual auto-focus algorithm that can drive a lens! > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/focus_status.h | 26 ++++++++++ > src/ipa/raspberrypi/controller/rpi/focus.cpp | 51 +++++++++++++++++++ > src/ipa/raspberrypi/controller/rpi/focus.hpp | 31 +++++++++++ > src/ipa/raspberrypi/data/imx477.json | 4 ++ > src/ipa/raspberrypi/meson.build | 1 + > 5 files changed, 113 insertions(+) > create mode 100644 src/ipa/raspberrypi/controller/focus_status.h > create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.cpp > create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.hpp > > diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h > new file mode 100644 > index 00000000..3ad88777 > --- /dev/null > +++ b/src/ipa/raspberrypi/controller/focus_status.h > @@ -0,0 +1,26 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > + * > + * focus_status.h - focus measurement status > + */ > +#pragma once > + > +#include <linux/bcm2835-isp.h> > + > +// The focus algorithm should post the following structure into the image's > +// "focus.status" metadata. Recall that it's only reporting focus (contrast) > +// measurements, it's not driving any kind of auto-focus algorithm! > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +struct FocusStatus { > + int num; > + uint32_t focus_measures[FOCUS_REGIONS]; > +}; > + > +#ifdef __cplusplus > +} > +#endif > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp > new file mode 100644 > index 00000000..d6cdc4bf > --- /dev/null > +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp > @@ -0,0 +1,51 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > + * > + * focus.cpp - focus algorithm > + */ > +#include <stdint.h> > + > +#include "../focus_status.h" > +#include "../logging.hpp" > +#include "focus.hpp" > + > +using namespace RPi; > + > +#define NAME "rpi.focus" > + > +Focus::Focus(Controller *controller) > + : Algorithm(controller) > +{ > +} > + > +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; > + int i; > + for (i = 0; i < FOCUS_REGIONS; i++) > + 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); > + } > +} > + > +/* Register algorithm with the system. */ > +static Algorithm *Create(Controller *controller) > +{ > + return new Focus(controller); > +} > +static RegisterAlgorithm reg(NAME, &Create); > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.hpp b/src/ipa/raspberrypi/controller/rpi/focus.hpp > new file mode 100644 > index 00000000..d53401f7 > --- /dev/null > +++ b/src/ipa/raspberrypi/controller/rpi/focus.hpp > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: BSD-2-Clause */ > +/* > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > + * > + * focus.hpp - focus algorithm > + */ > +#pragma once > + > +#include "../algorithm.hpp" > +#include "../metadata.hpp" > + > +/* > + * The "focus" algorithm. All it does it print out a version of the > + * focus contrast measure; there is no actual auto-focus mechanism to > + * control. > + */ > + > +namespace RPi { > + > +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_; This introduces the following coverity warning: ** CID 293456: Uninitialized members (UNINIT_CTOR) /home/linuxembedded/iob/libcamera/libcamera-daily/src/ipa/raspberrypi/controller/rpi/focus.cpp: 20 in RPi::Focus::Focus(RPi::Controller *)()
Hi Kieran, On Thu, 25 Jun 2020 at 17:27, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi David, Naush, > > On 28/05/2020 15:51, Naushir Patuck wrote: > > From: David Plowman <david.plowman@raspberrypi.com> > > > > Adds FocusStatus to the image metadata, containing contrast measurements > > across the image. Optionally also prints a contrast measure to the > > console, to aid in manual adjustment of the lens. Note that it is not an > > actual auto-focus algorithm that can drive a lens! > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/focus_status.h | 26 ++++++++++ > > src/ipa/raspberrypi/controller/rpi/focus.cpp | 51 +++++++++++++++++++ > > src/ipa/raspberrypi/controller/rpi/focus.hpp | 31 +++++++++++ > > src/ipa/raspberrypi/data/imx477.json | 4 ++ > > src/ipa/raspberrypi/meson.build | 1 + > > 5 files changed, 113 insertions(+) > > create mode 100644 src/ipa/raspberrypi/controller/focus_status.h > > create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.cpp > > create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.hpp > > > > diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h > > new file mode 100644 > > index 00000000..3ad88777 > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/focus_status.h > > @@ -0,0 +1,26 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > > + * > > + * focus_status.h - focus measurement status > > + */ > > +#pragma once > > + > > +#include <linux/bcm2835-isp.h> > > + > > +// The focus algorithm should post the following structure into the image's > > +// "focus.status" metadata. Recall that it's only reporting focus (contrast) > > +// measurements, it's not driving any kind of auto-focus algorithm! > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +struct FocusStatus { > > + int num; > > + uint32_t focus_measures[FOCUS_REGIONS]; > > +}; > > + > > +#ifdef __cplusplus > > +} > > +#endif > > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp > > new file mode 100644 > > index 00000000..d6cdc4bf > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp > > @@ -0,0 +1,51 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > > + * > > + * focus.cpp - focus algorithm > > + */ > > +#include <stdint.h> > > + > > +#include "../focus_status.h" > > +#include "../logging.hpp" > > +#include "focus.hpp" > > + > > +using namespace RPi; > > + > > +#define NAME "rpi.focus" > > + > > +Focus::Focus(Controller *controller) > > + : Algorithm(controller) > > +{ > > +} > > + > > +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; > > + int i; > > + for (i = 0; i < FOCUS_REGIONS; i++) > > + 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); > > + } > > +} > > + > > +/* Register algorithm with the system. */ > > +static Algorithm *Create(Controller *controller) > > +{ > > + return new Focus(controller); > > +} > > +static RegisterAlgorithm reg(NAME, &Create); > > diff --git a/src/ipa/raspberrypi/controller/rpi/focus.hpp b/src/ipa/raspberrypi/controller/rpi/focus.hpp > > new file mode 100644 > > index 00000000..d53401f7 > > --- /dev/null > > +++ b/src/ipa/raspberrypi/controller/rpi/focus.hpp > > @@ -0,0 +1,31 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > +/* > > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited > > + * > > + * focus.hpp - focus algorithm > > + */ > > +#pragma once > > + > > +#include "../algorithm.hpp" > > +#include "../metadata.hpp" > > + > > +/* > > + * The "focus" algorithm. All it does it print out a version of the > > + * focus contrast measure; there is no actual auto-focus mechanism to > > + * control. > > + */ > > + > > +namespace RPi { > > + > > +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_; > > This introduces the following coverity warning: > > > ** CID 293456: Uninitialized members (UNINIT_CTOR) > /home/linuxembedded/iob/libcamera/libcamera-daily/src/ipa/raspberrypi/controller/rpi/focus.cpp: > 20 in RPi::Focus::Focus(RPi::Controller *)() > > > ________________________________________________________________________________________________________ > *** CID 293456: Uninitialized members (UNINIT_CTOR) > /home/linuxembedded/iob/libcamera/libcamera-daily/src/ipa/raspberrypi/controller/rpi/focus.cpp: > 20 in RPi::Focus::Focus(RPi::Controller *)() > 14 > 15 #define NAME "rpi.focus" > 16 > 17 Focus::Focus(Controller *controller) > 18 : Algorithm(controller) > 19 { > >>> CID 293456: Uninitialized members (UNINIT_CTOR) > >>> Non-static class member "print_" is not initialized in this > constructor nor in any functions that it calls. > 20 } > 21 > 22 char const *Focus::Name() const > 23 { > 24 return NAME; > 25 } > > If you supply a patch to fix, please add the tag: > > Reported-by: Coverity CID=293456 > > > Alternatively, if you believe it's a false positive and should not be > fixed, let me know and I'll mark it as such to close the issue. > Thanks for the heads-up. This is not actually a problem, as Focus::Read() initialises print_ and is always called before focus::process(). Not surprisingly, Coverity did not spot that. I'll put in an explicit initialiser to fix the defect. I'm about to push a set of patches for focus, so I'll add the fix to that. Regards, Naush
Hi Naush, On 26/06/2020 08:33, Naushir Patuck wrote: > Hi Kieran, > > On Thu, 25 Jun 2020 at 17:27, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: >> >> Hi David, Naush, >> >> On 28/05/2020 15:51, Naushir Patuck wrote: >>> From: David Plowman <david.plowman@raspberrypi.com> >>> >>> Adds FocusStatus to the image metadata, containing contrast measurements >>> across the image. Optionally also prints a contrast measure to the >>> console, to aid in manual adjustment of the lens. Note that it is not an >>> actual auto-focus algorithm that can drive a lens! >>> >>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >>> --- >>> src/ipa/raspberrypi/controller/focus_status.h | 26 ++++++++++ >>> src/ipa/raspberrypi/controller/rpi/focus.cpp | 51 +++++++++++++++++++ >>> src/ipa/raspberrypi/controller/rpi/focus.hpp | 31 +++++++++++ >>> src/ipa/raspberrypi/data/imx477.json | 4 ++ >>> src/ipa/raspberrypi/meson.build | 1 + >>> 5 files changed, 113 insertions(+) >>> create mode 100644 src/ipa/raspberrypi/controller/focus_status.h >>> create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.cpp >>> create mode 100644 src/ipa/raspberrypi/controller/rpi/focus.hpp >>> >>> diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h >>> new file mode 100644 >>> index 00000000..3ad88777 >>> --- /dev/null >>> +++ b/src/ipa/raspberrypi/controller/focus_status.h >>> @@ -0,0 +1,26 @@ >>> +/* SPDX-License-Identifier: BSD-2-Clause */ >>> +/* >>> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited >>> + * >>> + * focus_status.h - focus measurement status >>> + */ >>> +#pragma once >>> + >>> +#include <linux/bcm2835-isp.h> >>> + >>> +// The focus algorithm should post the following structure into the image's >>> +// "focus.status" metadata. Recall that it's only reporting focus (contrast) >>> +// measurements, it's not driving any kind of auto-focus algorithm! >>> + >>> +#ifdef __cplusplus >>> +extern "C" { >>> +#endif >>> + >>> +struct FocusStatus { >>> + int num; >>> + uint32_t focus_measures[FOCUS_REGIONS]; >>> +}; >>> + >>> +#ifdef __cplusplus >>> +} >>> +#endif >>> diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp >>> new file mode 100644 >>> index 00000000..d6cdc4bf >>> --- /dev/null >>> +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp >>> @@ -0,0 +1,51 @@ >>> +/* SPDX-License-Identifier: BSD-2-Clause */ >>> +/* >>> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited >>> + * >>> + * focus.cpp - focus algorithm >>> + */ >>> +#include <stdint.h> >>> + >>> +#include "../focus_status.h" >>> +#include "../logging.hpp" >>> +#include "focus.hpp" >>> + >>> +using namespace RPi; >>> + >>> +#define NAME "rpi.focus" >>> + >>> +Focus::Focus(Controller *controller) >>> + : Algorithm(controller) >>> +{ >>> +} >>> + >>> +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; >>> + int i; >>> + for (i = 0; i < FOCUS_REGIONS; i++) >>> + 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); >>> + } >>> +} >>> + >>> +/* Register algorithm with the system. */ >>> +static Algorithm *Create(Controller *controller) >>> +{ >>> + return new Focus(controller); >>> +} >>> +static RegisterAlgorithm reg(NAME, &Create); >>> diff --git a/src/ipa/raspberrypi/controller/rpi/focus.hpp b/src/ipa/raspberrypi/controller/rpi/focus.hpp >>> new file mode 100644 >>> index 00000000..d53401f7 >>> --- /dev/null >>> +++ b/src/ipa/raspberrypi/controller/rpi/focus.hpp >>> @@ -0,0 +1,31 @@ >>> +/* SPDX-License-Identifier: BSD-2-Clause */ >>> +/* >>> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited >>> + * >>> + * focus.hpp - focus algorithm >>> + */ >>> +#pragma once >>> + >>> +#include "../algorithm.hpp" >>> +#include "../metadata.hpp" >>> + >>> +/* >>> + * The "focus" algorithm. All it does it print out a version of the >>> + * focus contrast measure; there is no actual auto-focus mechanism to >>> + * control. >>> + */ >>> + >>> +namespace RPi { >>> + >>> +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_; >> >> This introduces the following coverity warning: >> >> >> ** CID 293456: Uninitialized members (UNINIT_CTOR) >> /home/linuxembedded/iob/libcamera/libcamera-daily/src/ipa/raspberrypi/controller/rpi/focus.cpp: >> 20 in RPi::Focus::Focus(RPi::Controller *)() >> >> >> ________________________________________________________________________________________________________ >> *** CID 293456: Uninitialized members (UNINIT_CTOR) >> /home/linuxembedded/iob/libcamera/libcamera-daily/src/ipa/raspberrypi/controller/rpi/focus.cpp: >> 20 in RPi::Focus::Focus(RPi::Controller *)() >> 14 >> 15 #define NAME "rpi.focus" >> 16 >> 17 Focus::Focus(Controller *controller) >> 18 : Algorithm(controller) >> 19 { >>>>> CID 293456: Uninitialized members (UNINIT_CTOR) >>>>> Non-static class member "print_" is not initialized in this >> constructor nor in any functions that it calls. >> 20 } >> 21 >> 22 char const *Focus::Name() const >> 23 { >> 24 return NAME; >> 25 } >> >> If you supply a patch to fix, please add the tag: >> >> Reported-by: Coverity CID=293456 >> >> >> Alternatively, if you believe it's a false positive and should not be >> fixed, let me know and I'll mark it as such to close the issue. >> > > Thanks for the heads-up. This is not actually a problem, as > Focus::Read() initialises print_ and is always called before > focus::process(). Not surprisingly, Coverity did not spot that. I'll > put in an explicit initialiser to fix the defect. I'm about to push a > set of patches for focus, so I'll add the fix to that. Indeed, coverity does pick up a few false positives, particularly around these where it can't determine the execution order, so in those cases I'll just close them if the submitter choses. Coverity runs on a daily cron job, but only on the master branch, so I'll just send these reports manually until I can figure out how to automate it ;-) If you'd like an account to see the results, either sign up at https://scan.coverity.com/projects/libcamera or let me know and I can manually 'invite'. Ideally, we'll get some 'pre-integration' tests running so that we can catch things before they get merged too, but baby steps for now ... ;-) -- Kieran
diff --git a/src/ipa/raspberrypi/controller/focus_status.h b/src/ipa/raspberrypi/controller/focus_status.h new file mode 100644 index 00000000..3ad88777 --- /dev/null +++ b/src/ipa/raspberrypi/controller/focus_status.h @@ -0,0 +1,26 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright (C) 2020, Raspberry Pi (Trading) Limited + * + * focus_status.h - focus measurement status + */ +#pragma once + +#include <linux/bcm2835-isp.h> + +// The focus algorithm should post the following structure into the image's +// "focus.status" metadata. Recall that it's only reporting focus (contrast) +// measurements, it's not driving any kind of auto-focus algorithm! + +#ifdef __cplusplus +extern "C" { +#endif + +struct FocusStatus { + int num; + uint32_t focus_measures[FOCUS_REGIONS]; +}; + +#ifdef __cplusplus +} +#endif diff --git a/src/ipa/raspberrypi/controller/rpi/focus.cpp b/src/ipa/raspberrypi/controller/rpi/focus.cpp new file mode 100644 index 00000000..d6cdc4bf --- /dev/null +++ b/src/ipa/raspberrypi/controller/rpi/focus.cpp @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright (C) 2020, Raspberry Pi (Trading) Limited + * + * focus.cpp - focus algorithm + */ +#include <stdint.h> + +#include "../focus_status.h" +#include "../logging.hpp" +#include "focus.hpp" + +using namespace RPi; + +#define NAME "rpi.focus" + +Focus::Focus(Controller *controller) + : Algorithm(controller) +{ +} + +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; + int i; + for (i = 0; i < FOCUS_REGIONS; i++) + 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); + } +} + +/* Register algorithm with the system. */ +static Algorithm *Create(Controller *controller) +{ + return new Focus(controller); +} +static RegisterAlgorithm reg(NAME, &Create); diff --git a/src/ipa/raspberrypi/controller/rpi/focus.hpp b/src/ipa/raspberrypi/controller/rpi/focus.hpp new file mode 100644 index 00000000..d53401f7 --- /dev/null +++ b/src/ipa/raspberrypi/controller/rpi/focus.hpp @@ -0,0 +1,31 @@ +/* SPDX-License-Identifier: BSD-2-Clause */ +/* + * Copyright (C) 2020, Raspberry Pi (Trading) Limited + * + * focus.hpp - focus algorithm + */ +#pragma once + +#include "../algorithm.hpp" +#include "../metadata.hpp" + +/* + * The "focus" algorithm. All it does it print out a version of the + * focus contrast measure; there is no actual auto-focus mechanism to + * control. + */ + +namespace RPi { + +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 dce5234f..389e8ce8 100644 --- a/src/ipa/raspberrypi/data/imx477.json +++ b/src/ipa/raspberrypi/data/imx477.json @@ -412,5 +412,9 @@ "rpi.sharpen": { + }, + "rpi.focus": + { + "print": 1 } } diff --git a/src/ipa/raspberrypi/meson.build b/src/ipa/raspberrypi/meson.build index 697902e9..9445cd09 100644 --- a/src/ipa/raspberrypi/meson.build +++ b/src/ipa/raspberrypi/meson.build @@ -29,6 +29,7 @@ rpi_ipa_sources = files([ 'controller/rpi/awb.cpp', 'controller/rpi/sharpen.cpp', 'controller/rpi/black_level.cpp', + 'controller/rpi/focus.cpp', 'controller/rpi/geq.cpp', 'controller/rpi/noise.cpp', 'controller/rpi/lux.cpp',