[libcamera-devel] [PATCH 09/10] android: camera_device: Use Camera properties for static Metadata

[Laurent Pinchart]

Hi Jacopo,

On Thu, Dec 05, 2019 at 03:40:17PM +0100, Jacopo Mondi wrote:
> On Wed, Dec 04, 2019 at 06:32:35PM +0200, Laurent Pinchart wrote:
> 
> [snip]
> 
> > > +	 */
> > >  	int32_t orientation = 0;
> > > -	staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION,
> > > -				  &orientation, 1);
> > > +	if (properties.contains(properties::Rotation))
> > > +		orientation = (360 - properties.get(properties::Rotation))
> >
> > This could be made slightly more efficient if you used .find() to avoid
> > the double lookup. Same below.
> >
> 
> I actually overlooked this one. We don't have a ControlList::find()
> operation, or at least we have one (const and non const versions) but
> it's rightfully private.
> 
> The find() we have do not return an iterator, as one would expect from
> a find() on an std::map, but they return the ControlValue associated
> with the numerical id of a ControlId, allowing the insertion of a new
> item in the non-const version (that's why they're rightfully private).
> 
> The only current pattern to 1) check if a control is on the control
> list and 2) get its value is the contains()+get() sequence implemented
> in thi patch, which, as you noticed, performs a double lookup.
> 
> I see a few ways to avoid this:
> 
> 1)
>   - rename the current find() operations
>   - provide public find() operations which return an iterator
>   - Explicitly call get<T> on iterator::second (which is a
>     ControlValue)
> 
>     auto it &ctrl = list.find(Control<>);
>     if (ctrl != list.end())
>             int32_t value = ctrl.second.get<int32_t>();
> 
> 2)
>   - make ControlList::get<>() return an iterator
>   - again explictly call get<T>() on the ControlValue returned as
>     iterator::second
> 
>     const auto &it = list.get(Control<>);
>     if (it != list.end)
>         int32_t value = it.second.get<int32_t>();
> 
> I'm not particularly thrilled by none of the two to be honest

I think we should fix this, but let's leave it as-is for now.

> > > +			    % 360;
> > > +	staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, &orientation, 1);
> > >
> > >  	std::vector<int32_t> testPatterModes = {
> > >  		ANDROID_SENSOR_TEST_PATTERN_MODE_OFF,
> > > @@ -310,6 +321,20 @@ camera_metadata_t *CameraDevice::getStaticMetadata()
> > >  				  lensApertures.size());
> > >
> > >  	uint8_t lensFacing = ANDROID_LENS_FACING_FRONT;
> > > +	if (properties.contains(properties::Location)) {
> > > +		int32_t location = properties.get(properties::Location);
> > > +		switch (location) {
> > > +		case CAMERA_LOCATION_FRONT:
> > > +			lensFacing = ANDROID_LENS_FACING_FRONT;
> > > +			break;
> > > +		case CAMERA_LOCATION_BACK:
> > > +			lensFacing = ANDROID_LENS_FACING_BACK;
> > > +			break;
> > > +		case CAMERA_LOCATION_EXTERNAL:
> > > +			lensFacing = ANDROID_LENS_FACING_EXTERNAL;
> > > +			break;
> > > +		}
> > > +	}
> > >  	staticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1);
> > >
> > >  	std::vector<float> lensFocalLenghts = {

-- 
Regards,

Laurent Pinchart