[libcamera-devel] [RFC PATCH 1/6] ipa: raspberrypi: agc: Remove using namespace in agc.hpp

[David Plowman]

Hi Hiro, Laurent

Thanks for this change.

On Tue, 5 Oct 2021 at 10:53, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> (CC'ing David and Naush)
>
> Thank you for the patch.
>
> On Tue, Oct 05, 2021 at 04:31:09PM +0900, Hirokazu Honda wrote:
> > "using namespace" in a header file propagates the namespace to
> > the files including the header file. So it should be avoided.
> > This removes "using namespace" in agc.hpp.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/agc.cpp | 7 +++++++
> >  src/ipa/raspberrypi/controller/rpi/agc.hpp | 6 ++----
> >  2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > index 289c1fce..f6a9cb0a 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp
> > @@ -22,6 +22,7 @@
> >  using namespace RPiController;
> >  using namespace libcamera;
> >  using libcamera::utils::Duration;
> > +using namespace std::literals::chrono_literals;
> >
> >  LOG_DEFINE_CATEGORY(RPiAgc)
> >
> > @@ -159,6 +160,12 @@ void AgcConfig::Read(boost::property_tree::ptree const &params)
> >       default_analogue_gain = params.get<double>("default_analogue_gain", 1.0);
> >  }
> >
> > +Agc::ExposureValues::ExposureValues()
> > +     : shutter(0s), analogue_gain(0),
> > +       total_exposure(0s), total_exposure_no_dg(0s)
> > +{
> > +}
> > +
> >  Agc::Agc(Controller *controller)
> >       : AgcAlgorithm(controller), metering_mode_(nullptr),
> >         exposure_mode_(nullptr), constraint_mode_(nullptr),
> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > index 82063636..c100d312 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp
> > @@ -24,8 +24,6 @@
> >
> >  namespace RPiController {
> >
> > -using namespace std::literals::chrono_literals;
> > -
> >  struct AgcMeteringMode {
> >       double weights[AGC_STATS_SIZE];
> >       void Read(boost::property_tree::ptree const &params);
> > @@ -112,8 +110,8 @@ private:
> >       uint64_t frame_count_;
> >       AwbStatus awb_;
> >       struct ExposureValues {
> > -             ExposureValues() : shutter(0s), analogue_gain(0),
> > -                                total_exposure(0s), total_exposure_no_dg(0s) {}
> > +             ExposureValues();
> > +
>
> I'm a bit more annoyed by this patch than the other ones in the series,
> as there's no way to use namespaces explicitly for literals, which leads
> to the constructor having to be de-inlined.
>
> As this header is used internally in the Raspberry Pi IPA I don't think
> the using directive is a big deal. I don't object to the change though.
> I'll let David and Naush decide.

I'm fine with this.

Actually, one could argue that we should get rid of agc.hpp and put
the class definition at the top of agc.cpp. Any external access to the
class is meant to be through the AgcAlgorithm base class (and the same
is true of most of our other algorithms). But that's probably one for
another day!

Best regards
David

>
> >               libcamera::utils::Duration shutter;
> >               double analogue_gain;
> >               libcamera::utils::Duration total_exposure;
>
> --
> Regards,
>
> Laurent Pinchart