[libcamera-devel,v2,1/6] android: camera_capabilities: Add messages for lack of FULL support
diff mbox series

Message ID 20211220232629.1485890-2-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • android: Miscellaneous fixes
Related show

Commit Message

Paul Elder Dec. 20, 2021, 11:26 p.m. UTC 
Print messages when some feature is missing that causes hardware level
FULL to not be supported.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/android/camera_capabilities.cpp | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi Dec. 21, 2021, 8:37 a.m. UTC | #1 
Hi Paul,

On Mon, Dec 20, 2021 at 05:26:24PM -0600, Paul Elder wrote:
> Print messages when some feature is missing that causes hardware level
> FULL to not be supported.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index c4c26089..6ae2cd4d 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -359,6 +359,9 @@ void CameraCapabilities::computeHwLevel(
>  {
>  	camera_metadata_ro_entry_t entry;
>  	bool found;
> +
You  can drop this empty line. I would move the noFull declaration at
the beginning of the function.

> +	const char *noFull = "Hardware level FULL unavailable: ";
> +

This should be a const std::string ?

However I see other similar construct using a char * already. Should
they be fixed ?

	const char *noMode = "Manual post processing capability unavailable: ";

>  	camera_metadata_enum_android_info_supported_hardware_level
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;
>
> @@ -372,8 +375,10 @@ void CameraCapabilities::computeHwLevel(
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
>
>  	found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);
> -	if (!found || *entry.data.i32 != 0)
> +	if (!found || *entry.data.i32 != 0) {
> +		LOG(HAL, Info) << noFull << "missing or invalid max sync latency";

Or you could simply drop noFull and add the string here.

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

Thanks
   j
>  		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> +	}
>
>  	hwLevel_ = hwLevel;
>  }
> --
> 2.27.0
>
Umang Jain Jan. 4, 2022, 3:33 a.m. UTC | #2 
Hi,

On 12/21/21 2:07 PM, Jacopo Mondi wrote:
> Hi Paul,
>
> On Mon, Dec 20, 2021 at 05:26:24PM -0600, Paul Elder wrote:
>> Print messages when some feature is missing that causes hardware level
>> FULL to not be supported.
>>
>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>> ---
>>   src/android/camera_capabilities.cpp | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
>> index c4c26089..6ae2cd4d 100644
>> --- a/src/android/camera_capabilities.cpp
>> +++ b/src/android/camera_capabilities.cpp
>> @@ -359,6 +359,9 @@ void CameraCapabilities::computeHwLevel(
>>   {
>>   	camera_metadata_ro_entry_t entry;
>>   	bool found;
>> +
> You  can drop this empty line. I would move the noFull declaration at
> the beginning of the function.
>
>> +	const char *noFull = "Hardware level FULL unavailable: ";
>> +
> This should be a const std::string ?
>
> However I see other similar construct using a char * already. Should
> they be fixed ?
>
> 	const char *noMode = "Manual post processing capability unavailable: ";
>
>>   	camera_metadata_enum_android_info_supported_hardware_level
>>   		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;
>>
>> @@ -372,8 +375,10 @@ void CameraCapabilities::computeHwLevel(
>>   		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
>>
>>   	found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);
>> -	if (!found || *entry.data.i32 != 0)
>> +	if (!found || *entry.data.i32 != 0) {
>> +		LOG(HAL, Info) << noFull << "missing or invalid max sync latency";
> Or you could simply drop noFull and add the string here.
>
> Either ways
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


Similar comments, with these addressed:

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

>
> Thanks
>     j
>>   		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
>> +	}
>>
>>   	hwLevel_ = hwLevel;
>>   }
>> --
>> 2.27.0
>>

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index c4c26089..6ae2cd4d 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -359,6 +359,9 @@  void CameraCapabilities::computeHwLevel(
 {
 	camera_metadata_ro_entry_t entry;
 	bool found;
+
+	const char *noFull = "Hardware level FULL unavailable: ";
+
 	camera_metadata_enum_android_info_supported_hardware_level
 		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_FULL;
 
@@ -372,8 +375,10 @@  void CameraCapabilities::computeHwLevel(
 		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
 
 	found = staticMetadata_->getEntry(ANDROID_SYNC_MAX_LATENCY, &entry);
-	if (!found || *entry.data.i32 != 0)
+	if (!found || *entry.data.i32 != 0) {
+		LOG(HAL, Info) << noFull << "missing or invalid max sync latency";
 		hwLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
+	}
 
 	hwLevel_ = hwLevel;
 }