[libcamera-devel] [PATCH v6 7/9] android: camera_device: Use the new CameraMetadata helper class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Sep 5 22:39:17 CEST 2019
Hi Jacopo,
On Thu, Sep 05, 2019 at 10:25:03PM +0200, Jacopo Mondi wrote:
> On Thu, Sep 05, 2019 at 01:02:59PM +0300, Laurent Pinchart wrote:
> > On Thu, Sep 05, 2019 at 11:52:05AM +0200, Jacopo Mondi wrote:
> >> On Thu, Sep 05, 2019 at 12:36:57PM +0300, Laurent Pinchart wrote:
> >>> On Thu, Sep 05, 2019 at 10:18:28AM +0200, Jacopo Mondi wrote:
> >>>> On Thu, Sep 05, 2019 at 09:47:34AM +0200, Jacopo Mondi wrote:
> >>>>> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>>>>
> >>>>> Simplify the implementation of metadata handling in the CameraDevice
> >>>>> class by using the new CameraMetadata helper class.
> >>>>>
> >>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>>>> ---
> >>>>> src/android/camera_device.cpp | 507 +++++++++++++---------------------
> >>>>> src/android/camera_device.h | 15 +-
> >>>>> 2 files changed, 198 insertions(+), 324 deletions(-)
> >>>>>
> >>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>>>> index 5f8d19b9ef3d..475fa2404421 100644
> >>>>> --- a/src/android/camera_device.cpp
> >>>>> +++ b/src/android/camera_device.cpp
> >>>>> @@ -7,10 +7,10 @@
> >>>>>
> >>>>> #include "camera_device.h"
> >>>>>
> >>>>> -#include <system/camera_metadata.h>
> >>>>> -
> >>>>> #include "log.h"
> >>>>> +#include "utils.h"
> >>>>>
> >>>>> +#include "camera_metadata.h"
> >>>>> #include "thread_rpc.h"
> >>>>>
> >>>>> using namespace libcamera;
> >>>>> @@ -59,12 +59,10 @@ CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camer
> >>>>> CameraDevice::~CameraDevice()
> >>>>> {
> >>>>> if (staticMetadata_)
> >>>>> - free_camera_metadata(staticMetadata_);
> >>>>> - staticMetadata_ = nullptr;
> >>>>> + delete staticMetadata_;
> >>>>>
> >>>>> if (requestTemplate_)
> >>>>> - free_camera_metadata(requestTemplate_);
> >>>>> - requestTemplate_ = nullptr;
> >>>>> + delete requestTemplate_;
> >>>>> }
> >>>>>
> >>>>> /*
> >>>>> @@ -117,10 +115,8 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
> >>>>> */
> >>>>> camera_metadata_t *CameraDevice::getStaticMetadata()
> >>>>> {
> >>>>> - int ret;
> >>>>> -
> >>>>> if (staticMetadata_)
> >>>>> - return staticMetadata_;
> >>>>> + return staticMetadata_->get();
> >>>>>
> >>>>> /*
> >>>>> * The here reported metadata are enough to implement a basic capture
> >>>>> @@ -132,16 +128,21 @@ camera_metadata_t *CameraDevice::getStaticMetadata()
> >>>>> * \todo Keep this in sync with the actual number of entries.
> >>>>> * Currently: 46 entries, 390 bytes
> >>>>> */
> >>>>> - staticMetadata_ = allocate_camera_metadata(50, 500);
> >>>>> + staticMetadata_ = new CameraMetadata(50, 500);
> >>>>> + if (!staticMetadata_->isValid()) {
> >>>>> + LOG(HAL, Error) << "Failed to allocate static metadata";
> >>>>> + delete staticMetadata_;
> >>>>> + staticMetadata_ = nullptr;
> >>>>> + return nullptr;
> >>>>
> >>>> Empty line before return?
> >>>
> >>> I don't mind either way, but I haven't added one as the delete,
> >>> assignment to nullptr and return nullptr are logically grouped in my
> >>> opinion. Feel free to add a blank line if you prefer.
> >>
> >> Ok, if this was intentional let's keep it the way it is.
> >>
> >>>>> + }
> >>>>>
> >>>>> /* Color correction static metadata. */
> >>>>> std::vector<uint8_t> aberrationModes = {
> >>>>> ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,
> >>>>> - aberrationModes.data(), aberrationModes.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_COLOR_CORRECTION_AVAILABLE_ABERRATION_MODES,
> >>>>> + aberrationModes.data(),
> >>>>> + aberrationModes.size());
> >>>>>
> >>>>> /* Control static metadata. */
> >>>>> std::vector<uint8_t> aeAvailableAntiBandingModes = {
> >>>>> @@ -150,294 +151,228 @@ camera_metadata_t *CameraDevice::getStaticMetadata()
> >>>>> ANDROID_CONTROL_AE_ANTIBANDING_MODE_60HZ,
> >>>>> ANDROID_CONTROL_AE_ANTIBANDING_MODE_AUTO,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> >>>>> - aeAvailableAntiBandingModes.data(),
> >>>>> - aeAvailableAntiBandingModes.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,
> >>>>> + aeAvailableAntiBandingModes.data(),
> >>>>> + aeAvailableAntiBandingModes.size());
> >>>>>
> >>>>> std::vector<uint8_t> aeAvailableModes = {
> >>>>> ANDROID_CONTROL_AE_MODE_ON,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AE_AVAILABLE_MODES,
> >>>>> - aeAvailableModes.data(), aeAvailableModes.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,
> >>>>> + aeAvailableModes.data(),
> >>>>> + aeAvailableModes.size());
> >>>>>
> >>>>> std::vector<int32_t> availableAeFpsTarget = {
> >>>>> 15, 30,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,
> >>>>> - availableAeFpsTarget.data(),
> >>>>> - availableAeFpsTarget.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES,
> >>>>> + availableAeFpsTarget.data(),
> >>>>> + availableAeFpsTarget.size());
> >>>>>
> >>>>> std::vector<int32_t> aeCompensationRange = {
> >>>>> 0, 0,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AE_COMPENSATION_RANGE,
> >>>>> - aeCompensationRange.data(),
> >>>>> - aeCompensationRange.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AE_COMPENSATION_RANGE,
> >>>>> + aeCompensationRange.data(),
> >>>>> + aeCompensationRange.size());
> >>>>>
> >>>>> const camera_metadata_rational_t aeCompensationStep[] = {
> >>>>> { 0, 1 }
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AE_COMPENSATION_STEP,
> >>>>> - aeCompensationStep, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AE_COMPENSATION_STEP,
> >>>>> + aeCompensationStep, 1);
> >>>>>
> >>>>> std::vector<uint8_t> availableAfModes = {
> >>>>> ANDROID_CONTROL_AF_MODE_OFF,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AF_AVAILABLE_MODES,
> >>>>> - availableAfModes.data(), availableAfModes.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AF_AVAILABLE_MODES,
> >>>>> + availableAfModes.data(),
> >>>>> + availableAfModes.size());
> >>>>>
> >>>>> std::vector<uint8_t> availableEffects = {
> >>>>> ANDROID_CONTROL_EFFECT_MODE_OFF,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AVAILABLE_EFFECTS,
> >>>>> - availableEffects.data(), availableEffects.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_EFFECTS,
> >>>>> + availableEffects.data(),
> >>>>> + availableEffects.size());
> >>>>>
> >>>>> std::vector<uint8_t> availableSceneModes = {
> >>>>> ANDROID_CONTROL_SCENE_MODE_DISABLED,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AVAILABLE_SCENE_MODES,
> >>>>> - availableSceneModes.data(), availableSceneModes.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_SCENE_MODES,
> >>>>> + availableSceneModes.data(),
> >>>>> + availableSceneModes.size());
> >>>>>
> >>>>> std::vector<uint8_t> availableStabilizationModes = {
> >>>>> ANDROID_CONTROL_VIDEO_STABILIZATION_MODE_OFF,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,
> >>>>> - availableStabilizationModes.data(),
> >>>>> - availableStabilizationModes.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_VIDEO_STABILIZATION_MODES,
> >>>>> + availableStabilizationModes.data(),
> >>>>> + availableStabilizationModes.size());
> >>>>>
> >>>>> std::vector<uint8_t> availableAwbModes = {
> >>>>> ANDROID_CONTROL_AWB_MODE_OFF,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AWB_AVAILABLE_MODES,
> >>>>> - availableAwbModes.data(), availableAwbModes.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AWB_AVAILABLE_MODES,
> >>>>> + availableAwbModes.data(),
> >>>>> + availableAwbModes.size());
> >>>>>
> >>>>> std::vector<int32_t> availableMaxRegions = {
> >>>>> 0, 0, 0,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_MAX_REGIONS,
> >>>>> - availableMaxRegions.data(), availableMaxRegions.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_MAX_REGIONS,
> >>>>> + availableMaxRegions.data(),
> >>>>> + availableMaxRegions.size());
> >>>>>
> >>>>> std::vector<uint8_t> sceneModesOverride = {
> >>>>> ANDROID_CONTROL_AE_MODE_ON,
> >>>>> ANDROID_CONTROL_AWB_MODE_AUTO,
> >>>>> ANDROID_CONTROL_AF_MODE_AUTO,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_SCENE_MODE_OVERRIDES,
> >>>>> - sceneModesOverride.data(), sceneModesOverride.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,
> >>>>> + sceneModesOverride.data(),
> >>>>> + sceneModesOverride.size());
> >>>>>
> >>>>> uint8_t aeLockAvailable = ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> >>>>> - &aeLockAvailable, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,
> >>>>> + &aeLockAvailable, 1);
> >>>>>
> >>>>> uint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> >>>>> - &awbLockAvailable, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,
> >>>>> + &awbLockAvailable, 1);
> >>>>>
> >>>>> char availableControlModes = ANDROID_CONTROL_MODE_AUTO;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_CONTROL_AVAILABLE_MODES,
> >>>>> - &availableControlModes, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_CONTROL_AVAILABLE_MODES,
> >>>>> + &availableControlModes, 1);
> >>>>>
> >>>>> /* JPEG static metadata. */
> >>>>> std::vector<int32_t> availableThumbnailSizes = {
> >>>>> 0, 0,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
> >>>>> - availableThumbnailSizes.data(),
> >>>>> - availableThumbnailSizes.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
> >>>>> + availableThumbnailSizes.data(),
> >>>>> + availableThumbnailSizes.size());
> >>>>>
> >>>>> /* Sensor static metadata. */
> >>>>> int32_t pixelArraySize[] = {
> >>>>> 2592, 1944,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> >>>>> - &pixelArraySize, 2);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> >>>>> + &pixelArraySize, 2);
> >>>>>
> >>>>> int32_t sensorSizes[] = {
> >>>>> 0, 0, 2560, 1920,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> >>>>> - &sensorSizes, 4);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> >>>>> + &sensorSizes, 4);
> >>>>>
> >>>>> int32_t sensitivityRange[] = {
> >>>>> 32, 2400,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SENSOR_INFO_SENSITIVITY_RANGE,
> >>>>> - &sensitivityRange, 2);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_SENSITIVITY_RANGE,
> >>>>> + &sensitivityRange, 2);
> >>>>>
> >>>>> uint16_t filterArr = ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT_GRBG;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> >>>>> - &filterArr, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> >>>>> + &filterArr, 1);
> >>>>>
> >>>>> int64_t exposureTimeRange[] = {
> >>>>> 100000, 200000000,
> >>>>> };
> >>>>
> >>>> I've been thinking for a while to stabilize on the use of std::vector in
> >>>> place of arrays, so e can always use .data() and .size() and have
> >>>> guaranteed we should only change the declaration and not the
> >>>> parameters to the addEntry() operation.
> >>>>
> >>>> This might be a good time to do so. What do you think?
> >>>
> >>> I've thought about it, and constructing a vector is a bit costly
> >>> compared to arrays. I'd rather used std::array<>, but that requires
> >>> specifying the size explicitly :-S I've been thinking about overloading
> >>> the addEntry() method, in order to be able to write
> >>>
> >>> staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> >>> { 100000, 200000000 });
> >>>
> >>> I'm not sure if that will be possible as we need to handle different
> >>> data types, but I think it's worth a try. Other overloaded methods
> >>> should also be added for the arrays of a single element:
> >>>
> >>> staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION, 0);
> >>>
> >>> I would prefer handling the standardisation of vector vs. array in a
> >>> separate patch, when dealing with overloaded versions of addEntry(), as
> >>> this patch is big enough already.
> >>
> >> Agreed
> >>
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> >>>>> - &exposureTimeRange, 2);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_EXPOSURE_TIME_RANGE,
> >>>>> + &exposureTimeRange, 2);
> >>>>>
> >>>>> int32_t orientation = 0;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SENSOR_ORIENTATION,
> >>>>> - &orientation, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SENSOR_ORIENTATION,
> >>>>> + &orientation, 1);
> >>>>
> >>>> Fits in 1 line
> >>>
> >>> I've kept it on two lines to use similar constructs through the whole
> >>> function, but I don't mind much, so please feel free to change it if you
> >>> prefer.
> >>
> >> If it was intentional, let's keep it the way it is
> >>
> >>>>>
> >>>>> std::vector<int32_t> testPatterModes = {
> >>>>> ANDROID_SENSOR_TEST_PATTERN_MODE_OFF,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
> >>>>> - testPatterModes.data(), testPatterModes.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
> >>>>> + testPatterModes.data(),
> >>>>> + testPatterModes.size());
> >>>>>
> >>>>> std::vector<float> physicalSize = {
> >>>>> 2592, 1944,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> >>>>> - physicalSize.data(), physicalSize.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PHYSICAL_SIZE,
> >>>>> + physicalSize.data(),
> >>>>> + physicalSize.size());
> >>>>
> >>>> Fits in 1 line
> >>>>
> >>>>>
> >>>>> uint8_t timestampSource = ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE_UNKNOWN;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,
> >>>>> - ×tampSource, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SENSOR_INFO_TIMESTAMP_SOURCE,
> >>>>> + ×tampSource, 1);
> >>>>>
> >>>>> /* Statistics static metadata. */
> >>>>> uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,
> >>>>> - &faceDetectMode, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_AVAILABLE_FACE_DETECT_MODES,
> >>>>> + &faceDetectMode, 1);
> >>>>>
> >>>>> int32_t maxFaceCount = 0;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_STATISTICS_INFO_MAX_FACE_COUNT,
> >>>>> - &maxFaceCount, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_STATISTICS_INFO_MAX_FACE_COUNT,
> >>>>> + &maxFaceCount, 1);
> >>>>>
> >>>>> /* Sync static metadata. */
> >>>>> int32_t maxLatency = ANDROID_SYNC_MAX_LATENCY_UNKNOWN;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SYNC_MAX_LATENCY, &maxLatency, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SYNC_MAX_LATENCY, &maxLatency, 1);
> >>>>>
> >>>>> /* Flash static metadata. */
> >>>>> char flashAvailable = ANDROID_FLASH_INFO_AVAILABLE_FALSE;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_FLASH_INFO_AVAILABLE,
> >>>>> - &flashAvailable, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_FLASH_INFO_AVAILABLE,
> >>>>> + &flashAvailable, 1);
> >>>>>
> >>>>> /* Lens static metadata. */
> >>>>> std::vector<float> lensApertures = {
> >>>>> 2.53 / 100,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_LENS_INFO_AVAILABLE_APERTURES,
> >>>>> - lensApertures.data(), lensApertures.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_LENS_INFO_AVAILABLE_APERTURES,
> >>>>> + lensApertures.data(),
> >>>>> + lensApertures.size());
> >>>>>
> >>>>> uint8_t lensFacing = ANDROID_LENS_FACING_FRONT;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_LENS_FACING, &lensFacing, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_LENS_FACING, &lensFacing, 1);
> >>>>>
> >>>>> std::vector<float> lensFocalLenghts = {
> >>>>> 1,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,
> >>>>> - lensFocalLenghts.data(),
> >>>>> - lensFocalLenghts.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_LENS_INFO_AVAILABLE_FOCAL_LENGTHS,
> >>>>> + lensFocalLenghts.data(),
> >>>>> + lensFocalLenghts.size());
> >>>>>
> >>>>> std::vector<uint8_t> opticalStabilizations = {
> >>>>> ANDROID_LENS_OPTICAL_STABILIZATION_MODE_OFF,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,
> >>>>> - opticalStabilizations.data(),
> >>>>> - opticalStabilizations.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_LENS_INFO_AVAILABLE_OPTICAL_STABILIZATION,
> >>>>> + opticalStabilizations.data(),
> >>>>> + opticalStabilizations.size());
> >>>>>
> >>>>> float hypeFocalDistance = 0;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,
> >>>>> - &hypeFocalDistance, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_LENS_INFO_HYPERFOCAL_DISTANCE,
> >>>>> + &hypeFocalDistance, 1);
> >>>>>
> >>>>> float minFocusDistance = 0;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,
> >>>>> - &minFocusDistance, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_LENS_INFO_MINIMUM_FOCUS_DISTANCE,
> >>>>> + &minFocusDistance, 1);
> >>>>>
> >>>>> /* Noise reduction modes. */
> >>>>> uint8_t noiseReductionModes = ANDROID_NOISE_REDUCTION_MODE_OFF;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> >>>>> - &noiseReductionModes, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_NOISE_REDUCTION_AVAILABLE_NOISE_REDUCTION_MODES,
> >>>>> + &noiseReductionModes, 1);
> >>>>>
> >>>>> /* Scaler static metadata. */
> >>>>> float maxDigitalZoom = 1;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
> >>>>> - &maxDigitalZoom, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MAX_DIGITAL_ZOOM,
> >>>>> + &maxDigitalZoom, 1);
> >>>>>
> >>>>> std::vector<uint32_t> availableStreamFormats = {
> >>>>> ANDROID_SCALER_AVAILABLE_FORMATS_BLOB,
> >>>>> ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888,
> >>>>> ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SCALER_AVAILABLE_FORMATS,
> >>>>> - availableStreamFormats.data(),
> >>>>> - availableStreamFormats.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_FORMATS,
> >>>>> + availableStreamFormats.data(),
> >>>>> + availableStreamFormats.size());
> >>>>>
> >>>>> std::vector<uint32_t> availableStreamConfigurations = {
> >>>>> ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920,
> >>>>> @@ -447,65 +382,57 @@ camera_metadata_t *CameraDevice::getStaticMetadata()
> >>>>> ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920,
> >>>>> ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,
> >>>>> - availableStreamConfigurations.data(),
> >>>>> - availableStreamConfigurations.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,
> >>>>> + availableStreamConfigurations.data(),
> >>>>> + availableStreamConfigurations.size());
> >>>>>
> >>>>> std::vector<int64_t> availableStallDurations = {
> >>>>> ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SCALER_AVAILABLE_STALL_DURATIONS,
> >>>>> - availableStallDurations.data(),
> >>>>> - availableStallDurations.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_STALL_DURATIONS,
> >>>>> + availableStallDurations.data(),
> >>>>> + availableStallDurations.size());
> >>>>>
> >>>>> std::vector<int64_t> minFrameDurations = {
> >>>>> ANDROID_SCALER_AVAILABLE_FORMATS_BLOB, 2560, 1920, 33333333,
> >>>>> ANDROID_SCALER_AVAILABLE_FORMATS_IMPLEMENTATION_DEFINED, 2560, 1920, 33333333,
> >>>>> ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888, 2560, 1920, 33333333,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,
> >>>>> - minFrameDurations.data(), minFrameDurations.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,
> >>>>> + minFrameDurations.data(),
> >>>>> + minFrameDurations.size());
> >>>>>
> >>>>> uint8_t croppingType = ANDROID_SCALER_CROPPING_TYPE_CENTER_ONLY;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_SCALER_CROPPING_TYPE, &croppingType, 1);
> >>>>>
> >>>>> /* Info static metadata. */
> >>>>> uint8_t supportedHWLevel = ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_LIMITED;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> >>>>> - &supportedHWLevel, 1);
> >>>>> + staticMetadata_->addEntry(ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL,
> >>>>> + &supportedHWLevel, 1);
> >>>>>
> >>>>> /* Request static metadata. */
> >>>>> int32_t partialResultCount = 1;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> >>>>> - &partialResultCount, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_REQUEST_PARTIAL_RESULT_COUNT,
> >>>>> + &partialResultCount, 1);
> >>>>>
> >>>>> uint8_t maxPipelineDepth = 2;
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> >>>>> - &maxPipelineDepth, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_REQUEST_PIPELINE_MAX_DEPTH,
> >>>>> + &maxPipelineDepth, 1);
> >>>>>
> >>>>> std::vector<uint8_t> availableCapabilities = {
> >>>>> ANDROID_REQUEST_AVAILABLE_CAPABILITIES_BACKWARD_COMPATIBLE,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(staticMetadata_,
> >>>>> - ANDROID_REQUEST_AVAILABLE_CAPABILITIES,
> >>>>> - availableCapabilities.data(),
> >>>>> - availableCapabilities.size());
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + staticMetadata_->addEntry(ANDROID_REQUEST_AVAILABLE_CAPABILITIES,
> >>>>> + availableCapabilities.data(),
> >>>>> + availableCapabilities.size());
> >>>>> +
> >>>>> + if (!staticMetadata_->isValid()) {
> >>>>> + delete staticMetadata_;
> >>>>> + staticMetadata_ = nullptr;
> >>>>> + return nullptr;
> >>>>> + }
> >>>>>
> >>>>> - return staticMetadata_;
> >>>>> + return staticMetadata_->get();
> >>>>> }
> >>>>>
> >>>>> /*
> >>>>> @@ -513,8 +440,6 @@ camera_metadata_t *CameraDevice::getStaticMetadata()
> >>>>> */
> >>>>> const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> >>>>> {
> >>>>> - int ret;
> >>>>> -
> >>>>> /*
> >>>>> * \todo Inspect type and pick the right metadata pack.
> >>>>> * As of now just use a single one for all templates.
> >>>>> @@ -545,89 +470,73 @@ const camera_metadata_t *CameraDevice::constructDefaultRequestSettings(int type)
> >>>>> }
> >>>>>
> >>>>> if (requestTemplate_)
> >>>>> - return requestTemplate_;
> >>>>> + return requestTemplate_->get();
> >>>>>
> >>>>> /*
> >>>>> * \todo Keep this in sync with the actual number of entries.
> >>>>> * Currently: 12 entries, 15 bytes
> >>>>> */
> >>>>> - requestTemplate_ = allocate_camera_metadata(15, 20);
> >>>>> + requestTemplate_ = new CameraMetadata(15, 20);
> >>>>> if (!requestTemplate_) {
> >>>>> LOG(HAL, Error) << "Failed to allocate template metadata";
> >>>>> + delete requestTemplate_;
> >>>>> + requestTemplate_ = nullptr;
> >>>>
> >>>> Empty line before return ?
> >>>>
> >>>>> return nullptr;
> >>>>> }
> >>>>>
> >>>>> uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_CONTROL_AE_MODE,
> >>>>> - &aeMode, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_CONTROL_AE_MODE,
> >>>>> + &aeMode, 1);
> >>>>
> >>>> This and many others entries here and below fit in 1 line.
> >>>>
> >>>>>
> >>>>> int32_t aeExposureCompensation = 0;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> >>>>> - &aeExposureCompensation, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,
> >>>>> + &aeExposureCompensation, 1);
> >>>>>
> >>>>> uint8_t aePrecaptureTrigger = ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER_IDLE;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> >>>>> - &aePrecaptureTrigger, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER,
> >>>>> + &aePrecaptureTrigger, 1);
> >>>>>
> >>>>> uint8_t aeLock = ANDROID_CONTROL_AE_LOCK_OFF;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_CONTROL_AE_LOCK,
> >>>>> - &aeLock, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_CONTROL_AE_LOCK,
> >>>>> + &aeLock, 1);
> >>>>>
> >>>>> uint8_t afTrigger = ANDROID_CONTROL_AF_TRIGGER_IDLE;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_CONTROL_AF_TRIGGER,
> >>>>> - &afTrigger, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_CONTROL_AF_TRIGGER,
> >>>>> + &afTrigger, 1);
> >>>>>
> >>>>> uint8_t awbMode = ANDROID_CONTROL_AWB_MODE_AUTO;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_CONTROL_AWB_MODE,
> >>>>> - &awbMode, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_CONTROL_AWB_MODE,
> >>>>> + &awbMode, 1);
> >>>>>
> >>>>> uint8_t awbLock = ANDROID_CONTROL_AWB_LOCK_OFF;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_CONTROL_AWB_LOCK,
> >>>>> - &awbLock, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_CONTROL_AWB_LOCK,
> >>>>> + &awbLock, 1);
> >>>>>
> >>>>> uint8_t flashMode = ANDROID_FLASH_MODE_OFF;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_FLASH_MODE,
> >>>>> - &flashMode, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_FLASH_MODE,
> >>>>> + &flashMode, 1);
> >>>>>
> >>>>> uint8_t faceDetectMode = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_STATISTICS_FACE_DETECT_MODE,
> >>>>> - &faceDetectMode, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,
> >>>>> + &faceDetectMode, 1);
> >>>>>
> >>>>> uint8_t noiseReduction = ANDROID_NOISE_REDUCTION_MODE_OFF;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_NOISE_REDUCTION_MODE, &noiseReduction, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_NOISE_REDUCTION_MODE,
> >>>>> + &noiseReduction, 1);
> >>>>>
> >>>>> uint8_t aberrationMode = ANDROID_COLOR_CORRECTION_ABERRATION_MODE_OFF;
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_COLOR_CORRECTION_ABERRATION_MODE,
> >>>>> - &aberrationMode, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + requestTemplate_->addEntry(ANDROID_COLOR_CORRECTION_ABERRATION_MODE,
> >>>>> + &aberrationMode, 1);
> >>>>> +
> >>>>> + requestTemplate_->addEntry(ANDROID_CONTROL_CAPTURE_INTENT,
> >>>>> + &captureIntent, 1);
> >>>>>
> >>>>> - ret = add_camera_metadata_entry(requestTemplate_,
> >>>>> - ANDROID_CONTROL_CAPTURE_INTENT,
> >>>>> - &captureIntent, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + if (!requestTemplate_->isValid()) {
> >>>>> + delete requestTemplate_;
> >>>>> + requestTemplate_ = nullptr;
> >>>>
> >>>> You should return nullptr here, otherwise the below requestTemplate_->get();
> >>>> will de-reference a nullptr.
> >>>
> >>> Absolutely, my bad.
> >>>
> >>>>> + }
> >>>>>
> >>>>> - return requestTemplate_;
> >>>>> + return requestTemplate_->get();
> >>>>> }
> >>>>>
> >>>>> /*
> >>>>> @@ -799,7 +708,7 @@ void CameraDevice::requestComplete(Request *request,
> >>>>> {
> >>>>> Buffer *libcameraBuffer = buffers.begin()->second;
> >>>>> camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> >>>>> - camera_metadata_t *resultMetadata = nullptr;
> >>>>> + std::unique_ptr<CameraMetadata> resultMetadata;
> >>>>>
> >>>>> if (request->status() != Request::RequestComplete) {
> >>>>> LOG(HAL, Error) << "Request not succesfully completed: "
> >>>>> @@ -837,16 +746,12 @@ void CameraDevice::requestComplete(Request *request,
> >>>>> captureResult.partial_result = 1;
> >>>>> resultMetadata = getResultMetadata(descriptor->frameNumber,
> >>>>> libcameraBuffer->timestamp());
> >>>>> - captureResult.result = resultMetadata;
> >>>>> + captureResult.result = resultMetadata->get();
> >>>>> }
> >>>>>
> >>>>> callbacks_->process_capture_result(callbacks_, &captureResult);
> >>>>>
> >>>>> delete descriptor;
> >>>>> - if (resultMetadata)
> >>>>> - free_camera_metadata(resultMetadata);
> >>>>> -
> >>>>> - return;
> >>>>> }
> >>>>>
> >>>>> void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)
> >>>>> @@ -875,89 +780,63 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> >>>>> /*
> >>>>> * Produce a set of fixed result metadata.
> >>>>> */
> >>>>> -camera_metadata_t *CameraDevice::getResultMetadata(int frame_number,
> >>>>> - int64_t timestamp)
> >>>>> +std::unique_ptr<CameraMetadata> CameraDevice::getResultMetadata(int frame_number,
> >>>>
> >>>> Not a fan of smart pointers, you know already. How does this work?
> >>>> Because the compiler applies copy-elision or because of the move
> >>>> semantic applied on the return value?
> >>>
> >>> std::unique_ptr<> has an operator= that takes an rvalue reference to
> >>> another std::unique_ptr<>, and uses move semantic. As explained in
> >>> https://en.cppreference.com/w/cpp/language/value_category, an rvalue is
> >>> either a prvalue or an xvalue, and a prvalue can be "a function call or
> >>> an overloaded operator expression, whose return type is non-reference".
> >>> As far as I can tell, that's why we don't need an std::move() in the
> >>> caller.
> >>
> >> I see.. This seems to suggest both RVO and move semantic can play a
> >> role https://stackoverflow.com/a/46571941
> >>
> >> Ah, c++
> >>
> >>>> difficult to follow in my opinion.
> >>>
> >>> I think we should standardise as much as possible on std::unique_ptr<>
> >>> when passing ownership of a returned pointer. That's documented in
> >>> Documentation/coding-style.rst.
> >>
> >> Ack, let's keep it the way it is then.
> >>
> >>>>> + int64_t timestamp)
> >>>>> {
> >>>>> - int ret;
> >>>>> -
> >>>>> /*
> >>>>> * \todo Keep this in sync with the actual number of entries.
> >>>>> * Currently: 13 entries, 36 bytes
> >>>>> */
> >>>>> - camera_metadata_t *resultMetadata = allocate_camera_metadata(15, 50);
> >>>>> + std::unique_ptr<CameraMetadata> resultMetadata =
> >>>>> + utils::make_unique<CameraMetadata>(15, 50);
> >>>>> + if (!resultMetadata->isValid()) {
> >>>>> + LOG(HAL, Error) << "Failed to allocate static metadata";
> >>>>> + return nullptr;
> >>>>> + }
> >>>>>
> >>>>> const uint8_t ae_state = ANDROID_CONTROL_AE_STATE_CONVERGED;
> >>>>> - ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_STATE,
> >>>>> - &ae_state, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_CONTROL_AE_STATE, &ae_state, 1);
> >>>>>
> >>>>> const uint8_t ae_lock = ANDROID_CONTROL_AE_LOCK_OFF;
> >>>>> - ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AE_LOCK,
> >>>>> - &ae_lock, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, &ae_lock, 1);
> >>>>>
> >>>>> uint8_t af_state = ANDROID_CONTROL_AF_STATE_INACTIVE;
> >>>>> - ret = add_camera_metadata_entry(resultMetadata, ANDROID_CONTROL_AF_STATE,
> >>>>> - &af_state, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_CONTROL_AF_STATE, &af_state, 1);
> >>>>>
> >>>>> const uint8_t awb_state = ANDROID_CONTROL_AWB_STATE_CONVERGED;
> >>>>> - ret = add_camera_metadata_entry(resultMetadata,
> >>>>> - ANDROID_CONTROL_AWB_STATE,
> >>>>> - &awb_state, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_CONTROL_AWB_STATE, &awb_state, 1);
> >>>>>
> >>>>> const uint8_t awb_lock = ANDROID_CONTROL_AWB_LOCK_OFF;
> >>>>> - ret = add_camera_metadata_entry(resultMetadata,
> >>>>> - ANDROID_CONTROL_AWB_LOCK,
> >>>>> - &awb_lock, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_CONTROL_AWB_LOCK, &awb_lock, 1);
> >>>>>
> >>>>> const uint8_t lens_state = ANDROID_LENS_STATE_STATIONARY;
> >>>>> - ret = add_camera_metadata_entry(resultMetadata,
> >>>>> - ANDROID_LENS_STATE,
> >>>>> - &lens_state, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_LENS_STATE, &lens_state, 1);
> >>>>>
> >>>>> int32_t sensorSizes[] = {
> >>>>> 0, 0, 2560, 1920,
> >>>>> };
> >>>>> - ret = add_camera_metadata_entry(resultMetadata,
> >>>>> - ANDROID_SCALER_CROP_REGION,
> >>>>> - sensorSizes, 4);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_SCALER_CROP_REGION, sensorSizes, 4);
> >>>>>
> >>>>> - ret = add_camera_metadata_entry(resultMetadata,
> >>>>> - ANDROID_SENSOR_TIMESTAMP,
> >>>>> - ×tamp, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);
> >>>>>
> >>>>> /* 33.3 msec */
> >>>>> const int64_t rolling_shutter_skew = 33300000;
> >>>>> - ret = add_camera_metadata_entry(resultMetadata,
> >>>>> - ANDROID_SENSOR_ROLLING_SHUTTER_SKEW,
> >>>>> - &rolling_shutter_skew, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_SENSOR_ROLLING_SHUTTER_SKEW,
> >>>>> + &rolling_shutter_skew, 1);
> >>>>>
> >>>>> /* 16.6 msec */
> >>>>> const int64_t exposure_time = 16600000;
> >>>>> - ret = add_camera_metadata_entry(resultMetadata,
> >>>>> - ANDROID_SENSOR_EXPOSURE_TIME,
> >>>>> - &exposure_time, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME,
> >>>>> + &exposure_time, 1);
> >>>>>
> >>>>> const uint8_t lens_shading_map_mode =
> >>>>> ANDROID_STATISTICS_LENS_SHADING_MAP_MODE_OFF;
> >>>>> - ret = add_camera_metadata_entry(resultMetadata,
> >>>>> - ANDROID_STATISTICS_LENS_SHADING_MAP_MODE,
> >>>>> - &lens_shading_map_mode, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_STATISTICS_LENS_SHADING_MAP_MODE,
> >>>>> + &lens_shading_map_mode, 1);
> >>>>>
> >>>>> const uint8_t scene_flicker = ANDROID_STATISTICS_SCENE_FLICKER_NONE;
> >>>>> - ret = add_camera_metadata_entry(resultMetadata,
> >>>>> - ANDROID_STATISTICS_SCENE_FLICKER,
> >>>>> - &scene_flicker, 1);
> >>>>> - METADATA_ASSERT(ret);
> >>>>> + resultMetadata->addEntry(ANDROID_STATISTICS_SCENE_FLICKER,
> >>>>> + &scene_flicker, 1);
> >>>>
> >>>> Shouldn't you check if the metadata pack is valid ?
> >>>
> >>> Good point. Should we check here or in the caller ? As the caller would
> >>> then need a nullptr check I'm tempted to do so in the caller. Actually,
> >>> without a change to the code, the caller will operate correctly I
> >>> believe
> >>>
> >>> resultMetadata = getResultMetadata(descriptor->frameNumber,
> >>> libcameraBuffer->timestamp());
> >>> captureResult.result = resultMetadata->get();
> >>>
> >>> which will set captureResult.result to nullptr if resultMetadata is not
> >>> valid.
> >>
> >> Yeah, but in that case we should notify an error back to the stack.
> >
> > That makes sense. It should be a faily uncommon case though, is it would
> > be the result of a bug in our code, but bugs happen. I wonder what the
> > proper error recovery would be in that case.
> >
> >> What about:
> >>
> >> resultMetadata = getResultMetadata(descriptor->frameNumber,
> >> libcameraBuffer->timestamp());
> >> if (status == CAMERA3_BUFFER_STATUS_ERROR || !resultMetadata) {
> >> /* \todo Improve error handling. */
> >> notifyError(descriptor->frameNumber,
> >> descriptor->buffers[0].stream);
> >> } else {
> >> notifyShutter(descriptor->frameNumber,
> >> libcameraBuffer->timestamp());
> >>
> >> captureResult.partial_result = 1;
> >> captureResult.result = resultMetadata->get();
> >> }
> >>
> >> The drawback is that we're generating the result metadata even if
> >> status is set to CAMERA3_BUFFER_STATUS_ERROR.
> >
> > That I don't think we should do, as I expect the result metadata
> > generation will not be possible in case of CAMERA3_BUFFER_STATUS_ERROR
> > when we'll stop generating fake metadata.
> >
> > How about this ?
> >
> > if (status == CAMERA3_BUFFER_STATUS_OK) {
> > notifyShutter(descriptor->frameNumber,
> > libcameraBuffer->timestamp());
> >
> > captureResult.partial_result = 1;
> > resultMetadata = getResultMetadata(descriptor->frameNumber,
> > libcameraBuffer->timestamp());
> > captureResult.result = resultMetadata->get();
> > }
> >
> > if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {
> > /* \todo Improve error handling. */
> > notifyError(descriptor->frameNumber,
> > descriptor->buffers[0].stream);
> > }
> >
> > The last condition could be written if (!captureResult.result) as
> > CAMERA3_BUFFER_STATUS_ERROR implies !captureResult.result, but I'd
> > rather have both to make it explicit.
>
> The only difference I see here is that we send up a shutter event and
> the if metadata generation fails we send back an error.
>
> In the previous version was either an error or the shutter
> notification, not both.
>
> I can't tell if this makes any difference to the camera stack to be
> honest.
Neither can I, and this shouldn't happen in the first place :-) If you
want to avoid it,
if (status == CAMERA3_BUFFER_STATUS_OK) {
captureResult.partial_result = 1;
resultMetadata = getResultMetadata(descriptor->frameNumber,
libcameraBuffer->timestamp());
captureResult.result = resultMetadata->get();
}
if (status == CAMERA3_BUFFER_STATUS_OK && captureResult.result) {
notifyShutter(descriptor->frameNumber,
libcameraBuffer->timestamp());
} else {
/* \todo Improve error handling. */
notifyError(descriptor->frameNumber,
descriptor->buffers[0].stream);
}
> >>>>>
> >>>>> return resultMetadata;
> >>>>> }
> >>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >>>>> index 7897ba9dc5c7..65ba877a02ea 100644
> >>>>> --- a/src/android/camera_device.h
> >>>>> +++ b/src/android/camera_device.h
> >>>>> @@ -19,13 +19,7 @@
> >>>>>
> >>>>> #include "message.h"
> >>>>>
> >>>>> -#define METADATA_ASSERT(_r) \
> >>>>> - do { \
> >>>>> - if (!(_r)) break; \
> >>>>> - LOG(HAL, Error) << "Error: " << __func__ << ":" << __LINE__; \
> >>>>> - return nullptr; \
> >>>>> - } while(0);
> >>>>> -
> >>>>> +class CameraMetadata;
> >>>>> class ThreadRpc;
> >>>>>
> >>>>> class CameraDevice : public libcamera::Object
> >>>>> @@ -59,14 +53,15 @@ private:
> >>>>>
> >>>>> void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >>>>> void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
> >>>>> - camera_metadata_t *getResultMetadata(int frame_number, int64_t timestamp);
> >>>>> + std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,
> >>>>> + int64_t timestamp);
> >>>>>
> >>>>> bool running_;
> >>>>> std::shared_ptr<libcamera::Camera> camera_;
> >>>>> std::unique_ptr<libcamera::CameraConfiguration> config_;
> >>>>>
> >>>>> - camera_metadata_t *staticMetadata_;
> >>>>> - camera_metadata_t *requestTemplate_;
> >>>>> + CameraMetadata *staticMetadata_;
> >>>>> + CameraMetadata *requestTemplate_;
> >>>>> const camera3_callback_ops_t *callbacks_;
> >>>>> };
> >>>>>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list