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

[Kieran Bingham]

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 at jmondi.org>
>> Signed-off-by: Kieran Bingham <kieran.bingham at 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 at ideasonboard.com>
> 
>> +
>>  		config_.reset();
>>  		return -EINVAL;
>>  	case CameraConfiguration::Invalid:
> 

-- 
Regards
--
Kieran