[libcamera-devel] [PATCH][RFC/UNTESTED] android: camera_metadata: Track tags of each entry added
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Jul 3 13:20:34 CEST 2020
Hi Laurent,
On 03/07/2020 11:51, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Jul 03, 2020 at 10:49:51AM +0200, 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
>
> Then we should fix addEntry(), it uses a uint32_t.
>
>>> You should reserve() space in tags_ in the CameraMetadata constructor.
>>
>> Wouldn't this require manual pre-calculation as we do today ?
>
> It would, but we already do that pre-calculation :-) As the value is
> already passed to the constructor, we could use it.
Ok, I see the point, but this is part of a (not yet existing series) to
/remove/ those pre-calculations ;-)
If I get back to this I'll add the reserve, but it would (hopefully) get
removed again in the same series.
--
Kieran
>
>>> 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 ?
>
> Or an extra parameter to the constructor to tell if the metadata is
> static or dynamic ? (This should then be a CameraMetadataType enum, not
> a bool). In the dynamic case, we would skip the reserve() and
> push_back().
>
>>> 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
--
Kieran
More information about the libcamera-devel
mailing list