[libcamera-devel] [PATCH][RFC/UNTESTED] android: camera_metadata: Track tags of each entry added
Jacopo Mondi
jacopo at jmondi.org
Fri Jul 3 13:44:26 CEST 2020
Hi Laurent,
On Fri, Jul 03, 2020 at 01:57:38PM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Fri, Jul 03, 2020 at 12:12:48PM +0200, Jacopo Mondi wrote:
> > On Fri, Jul 03, 2020 at 10:05:46AM +0100, Kieran Bingham wrote:
> > > On 03/07/2020 09:49, Jacopo Mondi wrote:
> > > > On Fri, Jul 03, 2020 at 03:53:34AM +0300, Laurent Pinchart wrote:
> > > >> 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
> > >
> > > Looks like uint32_t is used everywhere, so that's probably the better
> > > type to use.
> >
> > uint32_t is used by the android metadata library, but tags types is
> > described as int32
> >
> > TYPE_INT32 = 1,
> >
> > Seems like that they chose INT instead of UINT just to describe the
> > type, but the tags are actually stored as unsigned ?
>
> That's for the value, not the tag itself, isn't it ?
>
right! I confused ht two /(0.0)\
> > > >>
> > > >> You should reserve() space in tags_ in the CameraMetadata constructor.
> > > >>
> > > > Wouldn't this require manual pre-calculation as we do today ?
> > >
> > > Indeed, that's what I'm trying to remove. We could preallocate a size to
> > > reduce likely hood of reallocations or such - but this might change
> > > quite a bit anyway...
> > >
> > > >> 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 ?
> > >
> > > I need to add more keys to the static data, so my main aim here is to
> > > automate the calculations required throughout. Otherwise, I fear this
> > > will go wrong quickly. I also fear that the calculations might already
> > > be wrong, and could potentially be the cause of crashes I experience
> > > with multi-stream support. However I haven't been able to confirm/deny
> > > that theory yet. (or maybe the valid flag already tracks if we were/were
> > > not able to add entries to the metadata successfully).
> >
> > If you're talking about the existing code, when I had not enough space
> > allocated, I noticed, it segfaults very early :)
>
> Not the best way to notice though :-)
>
Effective, for sure.
What I wanted to point out was to help Kieran in ruling out a wrong
space allocation issue, which when happened to me was quite noticeable!
> > > I have already been toying with subclassing CameraMetadata to make a
> > > CameraMetadataNull, which would allow programmatically identifying the
> > > sizes required, rather than manually.
> > >
> > > (A fake MetaData instance which just tracks how many tags/ how much data
> > > is added)
> > >
> > > Equally, I could pull the vector out of the class, and have a wrapper to
> > > addEntry() which tracks the tags, and just keep that in the
> > > getStaticMetadata() function:
> > >
> > > Class EntryTagTracker {
> > > EntryTagTracker(CameraMetadata *md) : md_(md) {};
> > >
> > > bool addEntry(uint32_t tag, const void *data, size_t data_count);
> > > {
> > > bool ret = md_->addEntry(tag, data, data_count);
> > > if (ret)
> > > tags_.push_back(tag);
> > > return ret;
> > > }
> > >
> > > const std::vector<int32_t> &tags() { return tags_; }
> > >
> > > private:
> > > CameraMetadata *md_;
> > > std::vector<int32_t> tags_;
> > > }
> > >
> > >
> > > I have a vision forming to try to automate collection of the Request and
> > > Result keys too, which would require a Null metadata object, and calling
> > > (adapted) functions to extract the tags used and start up time.
> > >
> > >
> > > In fact, pulling all that together, a fake metadata object which
> > > /stores/ the tags, and tracks the size and count would then handle all
> > > the requirements, so I might retry that path in a bit.
> >
> > I don't think that is has to be 'fake'.
> >
> > Ideally our CameraMetadata wrapper should be changed to store tags in
> > a temporary container, tracking their number and sizes (the most
> > elegant way to do so would be to to provide an addEntry() overloaded
> > on the type of a vector<T> first argument, so that you can receive an
> >
> > addEntry(uin32_t tag, std::vector<T> &data)
> >
> > Add the raw vector content in a temporary (and unfortunately
> > relocatable) storage space, and add them one by one to an actual
> > camera_metadata_t through an helper function like
> >
> > storeMetadata(camera_metadata_t *metadata)
> >
> > Not a 30 minutes job I fear
>
> For static metadata this is completely fine, we can make it more complex
> (in the CPU usage sense) with additional memory allocation to achieve a
> simpler API. We would store data in custom containers and generate an
> Android metadata buffer at the end. That would be my preference, but it
> will take a bit of time to develop. It could even allow us to ditch the
> Android metadata library.
>
> For dynamic metadata, it's a bit of a different story, as we want to
> minimize the memory allocations.
yes this should be considered for static metadata only. I'll let
Kieran consider if that's wroth the effort at this point..
>
> > > >>> bool valid_;
> > > >>> };
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list