[libcamera-devel,v6,3/8] ipa: raspberrypi: Add an operator<< to struct DeviceStatus
diff mbox series

Message ID 20210709145825.2943443-4-naush@raspberrypi.com
State Accepted
Headers show
Series
  • ipa: raspberrypi: Allow long exposure modes for imx477.
Related show

Commit Message

Naushir Patuck July 9, 2021, 2:58 p.m. UTC 
Add an operator<< overload to log all fields in DeviceStatus, and remove the
manual logging statements in the IPA and CamHelper.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper.cpp             |  5 +----
 src/ipa/raspberrypi/controller/device_status.h | 15 +++++++++++++++
 src/ipa/raspberrypi/raspberrypi.cpp            |  5 +----
 3 files changed, 17 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi July 9, 2021, 4:13 p.m. UTC | #1 
Hi Naush,

On Fri, Jul 09, 2021 at 03:58:20PM +0100, Naushir Patuck wrote:
> Add an operator<< overload to log all fields in DeviceStatus, and remove the
> manual logging statements in the IPA and CamHelper.

Library-wide we prefer the usage of toString() in order not to
overload operator<< in a non-trvial way (although this might be
considered trivial, as redirecting a variable to a stream has the
predictable effect of printing it out :)

However, this lives in your IPA, so up to you!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp             |  5 +----
>  src/ipa/raspberrypi/controller/device_status.h | 15 +++++++++++++++
>  src/ipa/raspberrypi/raspberrypi.cpp            |  5 +----
>  3 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index e6d2258c66d7..1ec3f03e1aa3 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -188,10 +188,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
>  	deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
>  	deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
>
> -	LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> -			   << deviceStatus.shutter_speed
> -			   << " Gain : "
> -			   << deviceStatus.analogue_gain;
> +	LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;
>
>  	metadata.Set("device.status", deviceStatus);
>  }
> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
> index 73df7ce228dd..e2511d19b96d 100644
> --- a/src/ipa/raspberrypi/controller/device_status.h
> +++ b/src/ipa/raspberrypi/controller/device_status.h
> @@ -6,6 +6,8 @@
>   */
>  #pragma once
>
> +#include <iostream>
> +
>  #include <libcamera/base/utils.h>
>
>  /*
> @@ -20,6 +22,19 @@ struct DeviceStatus {
>  	{
>  	}
>
> +	friend std::ostream &operator<<(std::ostream &out, const DeviceStatus &d)
> +	{
> +		using namespace libcamera; /* for the Duration operator<< overload */
> +
> +		out << "Exposure: " << d.shutter_speed
> +		    << " Gain: " << d.analogue_gain
> +		    << " Aperture: " << d.aperture
> +		    << " Lens: " << d.lens_position
> +		    << " Flash: " << d.flash_intensity;
> +
> +		return out;
> +	}
> +
>  	/* time shutter is open */
>  	libcamera::utils::Duration shutter_speed;
>  	double analogue_gain;
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 4d09a84f6532..f51c970befb5 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1019,10 +1019,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)
>  	deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
>  	deviceStatus.analogue_gain = helper_->Gain(gainCode);
>
> -	LOG(IPARPI, Debug) << "Metadata - Exposure : "
> -			   << deviceStatus.shutter_speed
> -			   << " Gain : "
> -			   << deviceStatus.analogue_gain;
> +	LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;
>
>  	rpiMetadata_.Set("device.status", deviceStatus);
>  }
> --
> 2.25.1
>
Laurent Pinchart July 11, 2021, 6:53 p.m. UTC | #2 
Hi Naush,

Thank you for the patch.

On Fri, Jul 09, 2021 at 03:58:20PM +0100, Naushir Patuck wrote:
> Add an operator<< overload to log all fields in DeviceStatus, and remove the
> manual logging statements in the IPA and CamHelper.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp             |  5 +----
>  src/ipa/raspberrypi/controller/device_status.h | 15 +++++++++++++++
>  src/ipa/raspberrypi/raspberrypi.cpp            |  5 +----
>  3 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index e6d2258c66d7..1ec3f03e1aa3 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -188,10 +188,7 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
>  	deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
>  	deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
>  
> -	LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> -			   << deviceStatus.shutter_speed
> -			   << " Gain : "
> -			   << deviceStatus.analogue_gain;
> +	LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;
>  
>  	metadata.Set("device.status", deviceStatus);
>  }
> diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
> index 73df7ce228dd..e2511d19b96d 100644
> --- a/src/ipa/raspberrypi/controller/device_status.h
> +++ b/src/ipa/raspberrypi/controller/device_status.h
> @@ -6,6 +6,8 @@
>   */
>  #pragma once
>  
> +#include <iostream>
> +
>  #include <libcamera/base/utils.h>
>  
>  /*
> @@ -20,6 +22,19 @@ struct DeviceStatus {
>  	{
>  	}
>  
> +	friend std::ostream &operator<<(std::ostream &out, const DeviceStatus &d)
> +	{
> +		using namespace libcamera; /* for the Duration operator<< overload */
> +
> +		out << "Exposure: " << d.shutter_speed
> +		    << " Gain: " << d.analogue_gain
> +		    << " Aperture: " << d.aperture
> +		    << " Lens: " << d.lens_position
> +		    << " Flash: " << d.flash_intensity;
> +
> +		return out;
> +	}

Does this function need to be inline ? Moving it to a new
device_status.cpp file would avoid duplicating the implementation in
multiple object files.

> +
>  	/* time shutter is open */
>  	libcamera::utils::Duration shutter_speed;
>  	double analogue_gain;
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 4d09a84f6532..f51c970befb5 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1019,10 +1019,7 @@ void IPARPi::fillDeviceStatus(const ControlList &sensorControls)
>  	deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
>  	deviceStatus.analogue_gain = helper_->Gain(gainCode);
>  
> -	LOG(IPARPI, Debug) << "Metadata - Exposure : "
> -			   << deviceStatus.shutter_speed
> -			   << " Gain : "
> -			   << deviceStatus.analogue_gain;
> +	LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;
>  
>  	rpiMetadata_.Set("device.status", deviceStatus);
>  }
Naushir Patuck July 12, 2021, 8:59 a.m. UTC | #3 
Hi Laurent,

Thank you for your review feedback.

On Sun, 11 Jul 2021 at 19:54, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Fri, Jul 09, 2021 at 03:58:20PM +0100, Naushir Patuck wrote:
> > Add an operator<< overload to log all fields in DeviceStatus, and remove
> the
> > manual logging statements in the IPA and CamHelper.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/cam_helper.cpp             |  5 +----
> >  src/ipa/raspberrypi/controller/device_status.h | 15 +++++++++++++++
> >  src/ipa/raspberrypi/raspberrypi.cpp            |  5 +----
> >  3 files changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > index e6d2258c66d7..1ec3f03e1aa3 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -188,10 +188,7 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> >       deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
> >       deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
> >
> > -     LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > -                        << deviceStatus.shutter_speed
> > -                        << " Gain : "
> > -                        << deviceStatus.analogue_gain;
> > +     LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;
> >
> >       metadata.Set("device.status", deviceStatus);
> >  }
> > diff --git a/src/ipa/raspberrypi/controller/device_status.h
> b/src/ipa/raspberrypi/controller/device_status.h
> > index 73df7ce228dd..e2511d19b96d 100644
> > --- a/src/ipa/raspberrypi/controller/device_status.h
> > +++ b/src/ipa/raspberrypi/controller/device_status.h
> > @@ -6,6 +6,8 @@
> >   */
> >  #pragma once
> >
> > +#include <iostream>
> > +
> >  #include <libcamera/base/utils.h>
> >
> >  /*
> > @@ -20,6 +22,19 @@ struct DeviceStatus {
> >       {
> >       }
> >
> > +     friend std::ostream &operator<<(std::ostream &out, const
> DeviceStatus &d)
> > +     {
> > +             using namespace libcamera; /* for the Duration operator<<
> overload */
> > +
> > +             out << "Exposure: " << d.shutter_speed
> > +                 << " Gain: " << d.analogue_gain
> > +                 << " Aperture: " << d.aperture
> > +                 << " Lens: " << d.lens_position
> > +                 << " Flash: " << d.flash_intensity;
> > +
> > +             return out;
> > +     }
>
> Does this function need to be inline ? Moving it to a new
> device_status.cpp file would avoid duplicating the implementation in
> multiple object files.
>

Not necessary to be inline, but it was small enough that I did not think it
would
matter.

I will send an update with this in its own cpp file.  Will also address
your other
feedback comments in the next update.

Regards,
Naush


> > +
> >       /* time shutter is open */
> >       libcamera::utils::Duration shutter_speed;
> >       double analogue_gain;
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 4d09a84f6532..f51c970befb5 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -1019,10 +1019,7 @@ void IPARPi::fillDeviceStatus(const ControlList
> &sensorControls)
> >       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> >       deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >
> > -     LOG(IPARPI, Debug) << "Metadata - Exposure : "
> > -                        << deviceStatus.shutter_speed
> > -                        << " Gain : "
> > -                        << deviceStatus.analogue_gain;
> > +     LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;
> >
> >       rpiMetadata_.Set("device.status", deviceStatus);
> >  }
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
index e6d2258c66d7..1ec3f03e1aa3 100644
--- a/src/ipa/raspberrypi/cam_helper.cpp
+++ b/src/ipa/raspberrypi/cam_helper.cpp
@@ -188,10 +188,7 @@  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
 	deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
 	deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
 
-	LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
-			   << deviceStatus.shutter_speed
-			   << " Gain : "
-			   << deviceStatus.analogue_gain;
+	LOG(IPARPI, Debug) << "Metadata updated - " << deviceStatus;
 
 	metadata.Set("device.status", deviceStatus);
 }
diff --git a/src/ipa/raspberrypi/controller/device_status.h b/src/ipa/raspberrypi/controller/device_status.h
index 73df7ce228dd..e2511d19b96d 100644
--- a/src/ipa/raspberrypi/controller/device_status.h
+++ b/src/ipa/raspberrypi/controller/device_status.h
@@ -6,6 +6,8 @@ 
  */
 #pragma once
 
+#include <iostream>
+
 #include <libcamera/base/utils.h>
 
 /*
@@ -20,6 +22,19 @@  struct DeviceStatus {
 	{
 	}
 
+	friend std::ostream &operator<<(std::ostream &out, const DeviceStatus &d)
+	{
+		using namespace libcamera; /* for the Duration operator<< overload */
+
+		out << "Exposure: " << d.shutter_speed
+		    << " Gain: " << d.analogue_gain
+		    << " Aperture: " << d.aperture
+		    << " Lens: " << d.lens_position
+		    << " Flash: " << d.flash_intensity;
+
+		return out;
+	}
+
 	/* time shutter is open */
 	libcamera::utils::Duration shutter_speed;
 	double analogue_gain;
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 4d09a84f6532..f51c970befb5 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -1019,10 +1019,7 @@  void IPARPi::fillDeviceStatus(const ControlList &sensorControls)
 	deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
 	deviceStatus.analogue_gain = helper_->Gain(gainCode);
 
-	LOG(IPARPI, Debug) << "Metadata - Exposure : "
-			   << deviceStatus.shutter_speed
-			   << " Gain : "
-			   << deviceStatus.analogue_gain;
+	LOG(IPARPI, Debug) << "Metadata - " << deviceStatus;
 
 	rpiMetadata_.Set("device.status", deviceStatus);
 }