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

[Jacopo Mondi]

Hi Laurent,

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


> > +			    % 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191205/3a8eac53/attachment.sig>