[libcamera-devel] [PATCH][RFC/UNTESTED] android: camera_metadata: Track tags of each entry added
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Jul 3 11:05:46 CEST 2020
Heya,
On 03/07/2020 09:49, Jacopo Mondi wrote:
> 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
Looks like uint32_t is used everywhere, so that's probably the better
type to use.
>>
>> 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).
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.
>>
>>> bool valid_;
>>> };
>>>
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list