[libcamera-devel] [PATCH 05/16] android: capabilities: Initialize camera state when building properties

[Umang Jain]

Hi Jacopo,


On 8/31/21 3:27 PM, Jacopo Mondi wrote:
> Hi Paul,
>
> On Tue, Aug 31, 2021 at 11:17:07AM +0900, Paul Elder wrote:
>> Hi Jacopo,
>>
>> On Fri, Aug 27, 2021 at 02:07:46PM +0200, Jacopo Mondi wrote:
>>> Now that building the list of supported stream configuration requires
>>> applying a configuration to the Camera, re-initialize the camera
>>> controls by applying a configuration generated for the Viewfinder stream
>>> role before building the list of static metadata.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> Looks good, but I have a question.
>>
>>> ---
>>>   src/android/camera_capabilities.cpp | 18 +++++++++++++++---
>>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
>>> index fdda90379ce2..723a4fd5a880 100644
>>> --- a/src/android/camera_capabilities.cpp
>>> +++ b/src/android/camera_capabilities.cpp
>>> @@ -394,11 +394,14 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera,
>>>   	}
>>>
>>>   	ret = initializeStreamConfigurations();
>>> -	camera_->release();
>>> -	if (ret)
>>> +	if (ret) {
>>> +		camera_->release();
>>>   		return ret;
>>> +	}
>>>
>>> -	return initializeStaticMetadata();
>>> +	ret = initializeStaticMetadata();
>>> +	camera_->release();
>>> +	return ret;
>>>   }
>>>
>>>   std::vector<Size>
>>> @@ -682,6 +685,15 @@ int CameraCapabilities::initializeStaticMetadata()
>>>   		return -EINVAL;
>>>   	}
>>>
>>> +	std::unique_ptr<CameraConfiguration> cameraConfig =
>>> +		camera_->generateConfiguration({ StreamRole::Viewfinder });
>>  From what I see, initializeStreamConfigurations() generates a
>> configuration for the StillCapture role. Is it fine that this uses a
> It does so only to retrieve the maximum resolution.
> A todo note reminds us that we should get that from a camera property
>
> 	 * Get the maximum output resolutions
> 	 * \todo Get this from the camera properties once defined
> 	 */
> 	std::unique_ptr<CameraConfiguration> cameraConfig =
> 		camera_->generateConfiguration({ StillCapture });
>
>> different role? (Or was the other one changed to Viewfinder where I
>> wasn't paying attention?)
> Well, the choice of the Viewfinder role is pretty arbitrary, but we
> have to define a 'mode' the camera should be set to when initializing
> the static metadata. We don't have the luxury of assuming much from
> the underlying camera, and we have to construct the static metadata
> from the camera properties (static) and the camera controls (change
> depending on the configuration).
>
> Before this series the Camera::controls where static as well, so the
> Camera didn't get configured at all during the static metadata
> initialization, and we relied on the PH-initialized ControlInfoMap.
>
> As we now need to apply a Configuration to the Camera to collect
> per-configuration control limits, we need to re-initialize them to a
> known default before inspecting them.
>
> I considered Viewfinder as a good default, but I'm open to other
> proposals.


I agree with the reasoning but also feels to me that there should be 
comment placed in the file to address this.

Up to you.

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

>
>>
>> Paul
>>
>>> +	int ret = camera_->configure(cameraConfig.get());
>>> +	if (ret) {
>>> +		LOG(HAL, Error) << "Failed to initialize the camera state";
>>> +		staticMetadata_.reset();
>>> +		return ret;
>>> +	}
>>> +
>>>   	const ControlInfoMap &controlsInfo = camera_->controls();
>>>   	const ControlList &properties = camera_->properties();
>>>
>>> --
>>> 2.32.0
>>>