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

[Jacopo Mondi]

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.

>
>
> 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
> >