[libcamera-devel] [PATCH][RFC/UNTESTED] android: camera_metadata: Track tags of each entry added

[Jacopo Mondi]

Hi Laurent, Kieran,

On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Thu, Jul 02, 2020 at 10:40:09PM +0100, Kieran Bingham wrote:
> > Provide automatic tracking of tags added to automatically report the
> > keys used for the entry:
> >  ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,
> >
> > This allows automatic addition of added keys without having to manually
> > maintain the list in the code base.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/android/camera_device.cpp   | 57 ++++-----------------------------
> >  src/android/camera_metadata.cpp |  4 ++-
> >  src/android/camera_metadata.h   |  4 +++
> >  3 files changed, 13 insertions(+), 52 deletions(-)
> >
> > Sending this, completely untested because ... that's how I roll, and I
> > wanted to know if this is a reasonable route to reduce maintainance
> > burden.
> >
> > A next step beyond this is also to consider a two-pass iteration on all
> > of the meta-data structures, where the first pass will determine the
> > number of entries, and bytes required, while the second pass will
> > actually populate the android metadata.
> >
> > Anythoughts on this? It would mean processing the entries twice, but
> > would stop the guessing game of 'is there enough memory allocated
> > here'...
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 5a3b4dfae6a0..de73c31ed3ea 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -721,58 +721,13 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  				  availableCapabilities.data(),
> >  				  availableCapabilities.size());
> >
> > -	std::vector<int32_t> availableCharacteristicsKeys = {
> > -		ANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,
> > -		ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> > -		ANDROID_CONTROL_AE_AVAILABLE_MODES,
> > -		ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,
> > -		ANDROID_CONTROL_AE_COMPENSATION_RANGE,
> > -		ANDROID_CONTROL_AE_COMPENSATION_STEP,
> > -		ANDROID_CONTROL_AF_AVAILABLE_MODES,
> > -		ANDROID_CONTROL_AVAILABLE_EFFECTS,
> > -		ANDROID_CONTROL_AVAILABLE_SCENE_MODES,
> > -		ANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,
> > -		ANDROID_CONTROL_AWB_AVAILABLE_MODES,
> > -		ANDROID_CONTROL_MAX_REGIONS,
> > -		ANDROID_CONTROL_SCENE_MODE_OVERRIDES,
> > -		ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> > -		ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> > -		ANDROID_CONTROL_AVAILABLE_MODES,
> > -		ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
> > -		ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> > -		ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> > -		ANDROID_SENSOR_INFO_SENSITIVITY_RANGE,
> > -		ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> > -		ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> > -		ANDROID_SENSOR_ORIENTATION,
> > -		ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
> > -		ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> > -		ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,
> > -		ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,
> > -		ANDROID_STATISTICS_INFO_MAX_FACE_COUNT,
> > -		ANDROID_SYNC_MAX_LATENCY,
> > -		ANDROID_FLASH_INFO_AVAILABLE,
> > -		ANDROID_LENS_INFO_AVAILABLE_APERTURES,
> > -		ANDROID_LENS_FACING,
> > -		ANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,
> > -		ANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,
> > -		ANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,
> > -		ANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,
> > -		ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> > -		ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
> > -		ANDROID_SCALER_AVAILABLE_FORMATS,
> > -		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,
> > -		ANDROID_SCALER_AVAILABLE_STALL_DURATIONS,
> > -		ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,
> > -		ANDROID_SCALER_CROPPING_TYPE,
> > -		ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> > -		ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> > -		ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> > -		ANDROID_REQUEST_AVAILABLE_CAPABILITIES,
> > -	};
> > +	/*
> > +	 * All tags added to staticMetadata_ to this point are added
> > +	 * as keys for the available characteristics.
> > +	 */
> >  	staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CHARACTERISTICS_KEYS,
> > -				  availableCharacteristicsKeys.data(),
> > -				  availableCharacteristicsKeys.size());
> > +				  staticMetadata_->tags().data(),
> > +				  staticMetadata_->tags().size());
> >
> >  	std::vector<int32_t> availableRequestKeys = {
> >  		ANDROID_CONTROL_AE_MODE,
> > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> > index 47b2e4ef117a..15b569aea52b 100644
> > --- a/src/android/camera_metadata.cpp
> > +++ b/src/android/camera_metadata.cpp
> > @@ -30,8 +30,10 @@ bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
> >  	if (!valid_)
> >  		return false;
> >
> > -	if (!add_camera_metadata_entry(metadata_, tag, data, count))
> > +	if (!add_camera_metadata_entry(metadata_, tag, data, count)) {
> > +		tags_.push_back(tag);
> >  		return true;
> > +	}
> >
> >  	const char *name = get_camera_metadata_tag_name(tag);
> >  	if (name)
> > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> > index 75a9d7066f31..a0e23119e68f 100644
> > --- a/src/android/camera_metadata.h
> > +++ b/src/android/camera_metadata.h
> > @@ -8,6 +8,7 @@
> >  #define __ANDROID_CAMERA_METADATA_H__
> >
> >  #include <stdint.h>
> > +#include <vector>
> >
> >  #include <system/camera_metadata.h>
> >
> > @@ -20,10 +21,13 @@ public:
> >  	bool isValid() { return valid_; }
> >  	bool addEntry(uint32_t tag, const void *data, size_t data_count);
> >
> > +	const std::vector<int32_t> &tags() { return tags_; }
> > +
> >  	camera_metadata_t *get();
> >
> >  private:
> >  	camera_metadata_t *metadata_;
> > +	std::vector<int32_t> tags_;
>
> Aren't tags unsigned ?

If I'm not mistaken Android uses int32_t

>
> You should reserve() space in tags_ in the CameraMetadata constructor.
>

Wouldn't this require manual pre-calculation as we do today ?

> As CameraMetadata is also used to report dynamic metadata, we will
> always add tags to the vector, even if they're only used for static
> metadata. Not very nice, given that we should try to minimize dynamic
> memory allocation during streaming :-S
>

That's my concern too.

I think it's acceptable to perform relocations while building the
static metadata at camera initialization time, but not for run time
usage. Maybe a different class just for static metadata would work
better ?

> I like the automation this brings though, so maybe we could find a
> different approach that would still bring the same improvement ?
>
> >  	bool valid_;
> >  };
> >
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel