[libcamera-devel] [PATCH][RFC/UNTESTED] android: camera_metadata: Track tags of each entry added
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 23 12:27:54 CEST 2020
Hi Kieran,
On Thu, Jul 23, 2020 at 10:26:58AM +0100, Kieran Bingham wrote:
> On 03/07/2020 12:47, Laurent Pinchart wrote:
> > On Fri, Jul 03, 2020 at 01:44:26PM +0200, Jacopo Mondi wrote:
> >> On Fri, Jul 03, 2020 at 01:57:38PM +0300, Laurent Pinchart wrote:
> >>> 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..
>
> I think I got confused where the feeling of this RFC went.
>
> I need to dynamically add keys to requests.
>
> For example, the JPEG result size would only be added to requests which
> performed a JPEG encode.
>
> So we can not with a fixed value determine in advance how many entries
> we will add, without going through the process of executing the code to
> decide.
>
> We could determine a 'larger/large enough' value in advance perhaps.
> Would that be more to your liking?
>
> That maximum size can be determined during the stage where the available
> keys are initialised anyway.
>
> Otherwise, I can not see a route forwards without making some kind of
> dynamic allocation ...
I see three options:
- Adding metadata to a dynamic container (e.g. std::map) and generating
the Android metadata blob at the end
- Running a first pass to compute the size of the metadata buffer and a
second pass to fill it.
- Calculating a worst case size at initialization time and using it to
allocate metadata buffers.
The first option requires dynamic allocation, the second option uses
more CPU time, and the third option uses more memory than strictly
necessary. The code structure would likely also be more or less clean
depending on which option is picked.
As stated before, for static metadata, extra CPU time and dynamic memory
allocation isn't much of an issue. I'd prioritize ease of use there, and
code sharing with the dynamic metadata case. For dynamic metadata, it
would be better to minimize dynamic allocations as much as possible.
Let's also not forget that some metadata tags store a variable number of
entries. That's mostly used for static metadata and controls, but a few
dynamic metadata tags are also affected (android.request.outputStreams,
and several of the android.statistics.* and android.tonemap.* tags). The
number of entries should however be either bounded (e.g.
android.statistics.faceIds is bounded by
android.statistics.info.maxFaceCount) or constant for a given
configuration (e.g. the number of entries in
android.statistics.histogram doesn't vary with each frame).
> > Another point to consider on this topic is the inclusion of the Android
> > metadata library in the libcamera sources. It's a small library,
> > available as a shared object on both Android and Chrome OS, but we have
> > pulled its source in libcamera in order to allow compile-tests without
> > depending on a Chrome OS or Android environment. There's no urgency at
> > this point, but I'd like to fix this. One way is to link against an
> > external library, at the expense of restricting compile-tests, but
> > another way would be to stop using it completely if we find a better
> > API.
> >
> >>>>>>>> bool valid_;
> >>>>>>>> };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list