[libcamera-devel] [PATCH][RFC/UNTESTED] android: camera_metadata: Track tags of each entry added
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Jul 23 11:26:58 CEST 2020
Hi Laurent,
On 03/07/2020 12:47, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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 ...
> 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
--
Kieran
More information about the libcamera-devel
mailing list