[libcamera-devel] [PATCH][RFC/UNTESTED] android: camera_metadata: Track tags of each entry added
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 3 12:51:58 CEST 2020
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.
> > 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,
Laurent Pinchart
More information about the libcamera-devel
mailing list