[libcamera-devel] [PATCH v2 7/9] android: camera_device: Use CameraMetadata wrapper in processControls

[Laurent Pinchart]

Hi Jacopo and Paul,

On Thu, Jan 21, 2021 at 12:36:08PM +0100, Jacopo Mondi wrote:
> On Thu, Jan 21, 2021 at 07:15:47PM +0900, Paul Elder wrote:
> > Use the CameraMetadata wrapper to access android request settings
> > instead of accessing camera_metadata_t directly.

This describes what the patch does, but should also tell why.

> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > New in v2
> > ---
> >  src/android/camera_device.cpp | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 16d5b472..49af221b 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1639,12 +1639,10 @@ int CameraDevice::processControls(camera3_capture_request_t *camera3Request,
> >  	 * \todo As soon as more controls are handled, this part should be
> >  	 * broke out to a dedicated function.
> >  	 */
> > -	const camera_metadata_t *camera3Settings = camera3Request->settings;
> > +	CameraMetadata camera3Settings(camera3Request->settings);
> 
> I think it is very unfortunate that we clone a full metadata pack just
> to inspect it. In [8/9] you clone the metadata at processRequest
> beginning, and you should be able to re-use it here.
> 
> The settings shall actually be cloned in the Camera3RequestDescriptor
> constructor, as it can be made to accept the full
> camera3_capture_request_t.

I like that, it will simplify the constructor of
Camera3RequestDescriptor to call it with just

	Camera3RequestDescriptor(camera_.get(), camera3Request)

> I have an implementation of that which I was waiting to rebase on your
> CameraMetadata change. What do you think if I break out your [2/9] and
> handle the conversion here ? It should be rather quick and if we
> fast-track it you can remove it from your series (I keep thinking you
> should have broke out the Camera3RequestDescriptor change in 6/9 from
> the handling of JPEG related metadata)

Sounds good to me ! Thanks Jacopo for helping.

> >  	camera_metadata_ro_entry_t entry;
> > -	int ret = find_camera_metadata_ro_entry(camera3Settings,
> > -						ANDROID_SCALER_CROP_REGION,
> > -						&entry);
> > -	if (!ret) {
> > +	int ret = camera3Settings.getEntry(ANDROID_SCALER_CROP_REGION, &entry);
> > +	if (ret) {
> >  		const int32_t *data = entry.data.i32;
> >  		Rectangle cropRegion = { data[0], data[1],
> >  					 static_cast<unsigned int>(data[2]),

-- 
Regards,

Laurent Pinchart