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

[paul.elder at ideasonboard.com]

Hi Jacopo,

On Tue, Aug 31, 2021 at 11:57:50AM +0200, 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.

Ah, okay, I see.


Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

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