[libcamera-devel,10/10] android: libcamera: add useful debug prints
diff mbox series

Message ID 20221027055515.321791-11-nicholas@rothemail.net
State Accepted
Headers show
Series
  • [libcamera-devel,01/10] ipa: workaround libcxx duration limitation
Related show

Commit Message

Nicolas Dufresne via libcamera-devel Oct. 27, 2022, 5:55 a.m. UTC 
From: Nicholas Roth <nicholas@rothemail.net>

I identified opportunities to make libcamera's log output easier to
understand while working to get it working on my Android device as a
HAL. These additional logging statements came out of that and will
hopefully prove useful to Android distribution maintainers with the same
goal as mine and to users who attempt to debug tools like Waydroid.

Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
---
 src/android/camera_capabilities.cpp | 12 +++++++++---
 src/android/camera_hal_manager.cpp  |  3 ++-
 src/libcamera/base/log.cpp          |  4 +++-
 src/libcamera/v4l2_subdevice.cpp    |  2 +-
 4 files changed, 15 insertions(+), 6 deletions(-)

Comments

Jacopo Mondi Oct. 27, 2022, 4:55 p.m. UTC | #1 
Hi Nicholas

On Thu, Oct 27, 2022 at 12:55:15AM -0500, Nicholas Roth via libcamera-devel wrote:
> From: Nicholas Roth <nicholas@rothemail.net>
>
> I identified opportunities to make libcamera's log output easier to
> understand while working to get it working on my Android device as a
> HAL. These additional logging statements came out of that and will
> hopefully prove useful to Android distribution maintainers with the same
> goal as mine and to users who attempt to debug tools like Waydroid.
>
> Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
> ---
>  src/android/camera_capabilities.cpp | 12 +++++++++---
>  src/android/camera_hal_manager.cpp  |  3 ++-
>  src/libcamera/base/log.cpp          |  4 +++-
>  src/libcamera/v4l2_subdevice.cpp    |  2 +-
>  4 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 64bd8dde..ef0d10d0 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -374,14 +374,20 @@ void CameraCapabilities::computeHwLevel(
>  	camera_metadata_enum_android_info_supported_hardware_level
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;
>
> -	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))
> +	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR)) {
> +		LOG(HAL, Info) << noFull << "missing manual sensor";
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +	}
>
> -	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))
> +	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING)) {
> +		LOG(HAL, Info) << noFull << "missing manual post processing";
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +	}
>
> -	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))
> +	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE)) {
> +		LOG(HAL, Info) << noFull << "missing burst capture";
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +	}

Fine with me if it makes things easier for you

>
>  	found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);
>  	if (!found || *entry.data.i32 != 0) {
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index 7512cc4e..7fac4e3f 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -140,7 +140,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>  	 */
>  	if (!isCameraExternal && !halConfig_.exists()) {
>  		LOG(HAL, Error)
> -			<< "HAL configuration file is mandatory for internal cameras";
> +			<< "HAL configuration file is mandatory for internal cameras."
> +			<< " Camera NOT loaded: \"" << cam->id() << "\"";

that capital "NOT" is a bit bold :)

If you want to add information I would maybe
                        << " Camera " << cam->id() << " failed to load";

I don't think you need "" around the camera id (we don't have it in
the other error messages)


>  		return;
>  	}
>
> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> index 55fbd7b0..88800158 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -625,8 +625,10 @@ void Logger::parseLogFile()
>  void Logger::parseLogLevels()
>  {
>  	const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS");
> -	if (!debug)
> +	if (!debug) {
> +		syslog(LOG_INFO, "Could not find LIBCAMERA_LOG_LEVELS in env");
>  		return;
> +	}

This doesn't seems something to report everytime the library is loaded

>
>  	for (const char *pair = debug; *debug != '\0'; pair = debug) {
>  		const char *comma = strchrnul(debug, ',');
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 15e8206a..f3e25b86 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -392,7 +392,7 @@ int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
>  	if (ret < 0) {
>  		LOG(V4L2, Error)
>  			<< "Unable to get rectangle " << target << " on pad "
> -			<< pad << ": " << strerror(-ret);
> +			<< pad << ": " << strerror(-ret) << ".";

Do we end all other error messages with a full stop ? Why this one
should ?

>  		return ret;
>  	}
>
> --
> 2.34.1
>
Nicholas Roth Oct. 27, 2022, 10:30 p.m. UTC | #2 
> Fine with me if it makes things easier for you
It makes the logs easier to understand for me. My hope is that the next
person who wants to know why their camera is "LIMITED" instead of "FULL"
will find this useful, just like I would have. This will also help diagnose
when people, e.g. on the Waydroid bug tracker, complain about not having a
"FULL" camera as a bug and maybe even prevent bugs like that (though
probably not).

t> hat capital "NOT" is a bit bold :)
> If you want to add information I would maybe
>                         << " Camera " << cam->id() << " failed to load";

> I don't think you need "" around the camera id (we don't have it in
> the other error messages)
> Made lower-case.
All good points. Changes made.

> This doesn't seems something to report everytime the library is loaded
Removed.

> Do we end all other error messages with a full stop ? Why this one
should ?
Good point. Removed the full stop.

On Thu, Oct 27, 2022 at 11:55 AM Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Nicholas
>
> On Thu, Oct 27, 2022 at 12:55:15AM -0500, Nicholas Roth via
> libcamera-devel wrote:
> > From: Nicholas Roth <nicholas@rothemail.net>
> >
> > I identified opportunities to make libcamera's log output easier to
> > understand while working to get it working on my Android device as a
> > HAL. These additional logging statements came out of that and will
> > hopefully prove useful to Android distribution maintainers with the same
> > goal as mine and to users who attempt to debug tools like Waydroid.
> >
> > Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
> > ---
> >  src/android/camera_capabilities.cpp | 12 +++++++++---
> >  src/android/camera_hal_manager.cpp  |  3 ++-
> >  src/libcamera/base/log.cpp          |  4 +++-
> >  src/libcamera/v4l2_subdevice.cpp    |  2 +-
> >  4 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/android/camera_capabilities.cpp
> b/src/android/camera_capabilities.cpp
> > index 64bd8dde..ef0d10d0 100644
> > --- a/src/android/camera_capabilities.cpp
> > +++ b/src/android/camera_capabilities.cpp
> > @@ -374,14 +374,20 @@ void CameraCapabilities::computeHwLevel(
> >       camera_metadata_enum_android_info_supported_hardware_level
> >               hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;
> >
> > -     if
> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))
> > +     if
> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR)) {
> > +             LOG(HAL, Info) << noFull << "missing manual sensor";
> >               hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > +     }
> >
> > -     if
> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))
> > +     if
> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))
> {
> > +             LOG(HAL, Info) << noFull << "missing manual post
> processing";
> >               hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > +     }
> >
> > -     if
> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))
> > +     if
> (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE)) {
> > +             LOG(HAL, Info) << noFull << "missing burst capture";
> >               hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> > +     }
>
> Fine with me if it makes things easier for you
>
> >
> >       found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY,
> &entry);
> >       if (!found || *entry.data.i32 != 0) {
> > diff --git a/src/android/camera_hal_manager.cpp
> b/src/android/camera_hal_manager.cpp
> > index 7512cc4e..7fac4e3f 100644
> > --- a/src/android/camera_hal_manager.cpp
> > +++ b/src/android/camera_hal_manager.cpp
> > @@ -140,7 +140,8 @@ void
> CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> >        */
> >       if (!isCameraExternal && !halConfig_.exists()) {
> >               LOG(HAL, Error)
> > -                     << "HAL configuration file is mandatory for
> internal cameras";
> > +                     << "HAL configuration file is mandatory for
> internal cameras."
> > +                     << " Camera NOT loaded: \"" << cam->id() << "\"";
>
> that capital "NOT" is a bit bold :)
>
> If you want to add information I would maybe
>                         << " Camera " << cam->id() << " failed to load";
>
> I don't think you need "" around the camera id (we don't have it in
> the other error messages)
>
>
> >               return;
> >       }
> >
> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > index 55fbd7b0..88800158 100644
> > --- a/src/libcamera/base/log.cpp
> > +++ b/src/libcamera/base/log.cpp
> > @@ -625,8 +625,10 @@ void Logger::parseLogFile()
> >  void Logger::parseLogLevels()
> >  {
> >       const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS");
> > -     if (!debug)
> > +     if (!debug) {
> > +             syslog(LOG_INFO, "Could not find LIBCAMERA_LOG_LEVELS in
> env");
> >               return;
> > +     }
>
> This doesn't seems something to report everytime the library is loaded
>
> >
> >       for (const char *pair = debug; *debug != '\0'; pair = debug) {
> >               const char *comma = strchrnul(debug, ',');
> > diff --git a/src/libcamera/v4l2_subdevice.cpp
> b/src/libcamera/v4l2_subdevice.cpp
> > index 15e8206a..f3e25b86 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -392,7 +392,7 @@ int V4L2Subdevice::getSelection(unsigned int pad,
> unsigned int target,
> >       if (ret < 0) {
> >               LOG(V4L2, Error)
> >                       << "Unable to get rectangle " << target << " on
> pad "
> > -                     << pad << ": " << strerror(-ret);
> > +                     << pad << ": " << strerror(-ret) << ".";
>
> Do we end all other error messages with a full stop ? Why this one
> should ?
>
> >               return ret;
> >       }
> >
> > --
> > 2.34.1
> >
>

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index 64bd8dde..ef0d10d0 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -374,14 +374,20 @@  void CameraCapabilities::computeHwLevel(
 	camera_metadata_enum_android_info_supported_hardware_level
 		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;
 
-	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR))
+	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_SENSOR)) {
+		LOG(HAL, Info) << noFull << "missing manual sensor";
 		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
+	}
 
-	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING))
+	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_MANUAL_POST_PROCESSING)) {
+		LOG(HAL, Info) << noFull << "missing manual post processing";
 		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
+	}
 
-	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE))
+	if (!caps.count(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BURST_CAPTURE)) {
+		LOG(HAL, Info) << noFull << "missing burst capture";
 		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
+	}
 
 	found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);
 	if (!found || *entry.data.i32 != 0) {
diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
index 7512cc4e..7fac4e3f 100644
--- a/src/android/camera_hal_manager.cpp
+++ b/src/android/camera_hal_manager.cpp
@@ -140,7 +140,8 @@  void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
 	 */
 	if (!isCameraExternal && !halConfig_.exists()) {
 		LOG(HAL, Error)
-			<< "HAL configuration file is mandatory for internal cameras";
+			<< "HAL configuration file is mandatory for internal cameras."
+			<< " Camera NOT loaded: \"" << cam->id() << "\"";
 		return;
 	}
 
diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
index 55fbd7b0..88800158 100644
--- a/src/libcamera/base/log.cpp
+++ b/src/libcamera/base/log.cpp
@@ -625,8 +625,10 @@  void Logger::parseLogFile()
 void Logger::parseLogLevels()
 {
 	const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS");
-	if (!debug)
+	if (!debug) {
+		syslog(LOG_INFO, "Could not find LIBCAMERA_LOG_LEVELS in env");
 		return;
+	}
 
 	for (const char *pair = debug; *debug != '\0'; pair = debug) {
 		const char *comma = strchrnul(debug, ',');
diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
index 15e8206a..f3e25b86 100644
--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -392,7 +392,7 @@  int V4L2Subdevice::getSelection(unsigned int pad, unsigned int target,
 	if (ret < 0) {
 		LOG(V4L2, Error)
 			<< "Unable to get rectangle " << target << " on pad "
-			<< pad << ": " << strerror(-ret);
+			<< pad << ": " << strerror(-ret) << ".";
 		return ret;
 	}