[libcamera-devel] [PATCH 10/10] android: libcamera: add useful debug prints

[Jacopo Mondi]

Hi Nicholas

On Thu, Oct 27, 2022 at 12:55:15AM -0500, Nicholas Roth via libcamera-devel wrote:
> From: Nicholas Roth <nicholas at 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 at 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
>