[libcamera-devel,v3,10/13] android: camera_device: Report configuration changes from validate()

9203 diff mbox series

When we call validate on a configuration, if there are any adjustments
on the configuration, we fail without showing why.

Display the stream configuration after the validate stage to aid
debugging stream startup failures.

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/android/camera_device.cpp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Hi Kieran,

Thank you for the patch.

On Tue, Aug 04, 2020 at 10:47:08PM +0100, Kieran Bingham wrote:
> When we call validate on a configuration, if there are any adjustments
> on the configuration, we fail without showing why.
> 
> Display the stream configuration after the validate stage to aid
> debugging stream startup failures.
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 5899154b3e78..4178db952846 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1047,7 +1047,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	case CameraConfiguration::Valid:
>  		break;
>  	case CameraConfiguration::Adjusted:
> -		LOG(HAL, Info) << "Camera configuration adjusted";
> +		LOG(HAL, Info) << "Camera configuration adjusted:";
> +
> +		for (const StreamConfiguration &cfg : *config_)
> +			LOG(HAL, Info) << " : " << cfg.toString();

This would output

Camera configuration ajusted:
 : 1920x1080-NV12
 : 800x600-YUYV

The leading colons seem a bit weird to me, are they intended ? Would

Camera configuration ajusted:
- 1920x1080-NV12
- 800x600-YUYV

be better ?

You could also use the Debug level to log the stream configurations (but
as we're not supposed to fail, Info could be fine), in which case I'd
remove the colon as the end of the first line.

With or without any of those changes,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  		config_.reset();
>  		return -EINVAL;
>  	case CameraConfiguration::Invalid:

Hi Laurent,

On 05/08/2020 01:46, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Tue, Aug 04, 2020 at 10:47:08PM +0100, Kieran Bingham wrote:
>> When we call validate on a configuration, if there are any adjustments
>> on the configuration, we fail without showing why.
>>
>> Display the stream configuration after the validate stage to aid
>> debugging stream startup failures.
>>
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/android/camera_device.cpp | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 5899154b3e78..4178db952846 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1047,7 +1047,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  	case CameraConfiguration::Valid:
>>  		break;
>>  	case CameraConfiguration::Adjusted:
>> -		LOG(HAL, Info) << "Camera configuration adjusted";
>> +		LOG(HAL, Info) << "Camera configuration adjusted:";
>> +
>> +		for (const StreamConfiguration &cfg : *config_)
>> +			LOG(HAL, Info) << " : " << cfg.toString();
> 
> This would output
> 
> Camera configuration ajusted:
>  : 1920x1080-NV12
>  : 800x600-YUYV
> 
> The leading colons seem a bit weird to me, are they intended ? Would

The colons are a leftover from when I was able to display the associated
android stream index before the stream configuration.

I removed that because I now iterate the StreamConfigurations directly.

A hyphen makes more sense yes.


> 
> Camera configuration ajusted:
> - 1920x1080-NV12
> - 800x600-YUYV
> 
> be better ?
> 
> You could also use the Debug level to log the stream configurations (but
> as we're not supposed to fail, Info could be fine), in which case I'd
> remove the colon as the end of the first line.

I added the colon at the end because it indicates that the lines
following are a continuation of this statement.

In an earlier review Jacopo also suggested making these Debug, but I
would almost prefer to go the other way, and make all three lines 'Error'...

If the camera configuration is adjusted, then the android configuration
is going to fail, and it would be really helpful to know why ;-)


> With or without any of those changes,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +
>>  		config_.reset();
>>  		return -EINVAL;
>>  	case CameraConfiguration::Invalid:
>