| Message ID | 20220711003918.535-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thank you for the patch. On 7/11/22 06:09, Laurent Pinchart via libcamera-devel wrote: > Now that the ControlList::get() function returns an instance of > std::optional<>, we can replace the ControlList::contains() calls with a > nullopt check on the return value of get(). This avoids double lookups > of controls through the code base. The patch looks good to me, but I had a follow-up question whether the change of ControlList::get() returning a std::optional<> means we can drop .contains() helper probably? What do you think? > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > This patch applies on top of "[PATCH v10 1/2] libcamera: controls: Use > std::optional to handle invalid control values". > --- > src/android/camera_capabilities.cpp | 11 ++--- > src/android/camera_device.cpp | 46 +++++++++---------- > src/android/camera_hal_manager.cpp | 6 +-- > src/cam/main.cpp | 5 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 25 +++++----- > .../pipeline/raspberrypi/raspberrypi.cpp | 14 +++--- > src/qcam/dng_writer.cpp | 35 +++++++------- > 7 files changed, 68 insertions(+), 74 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 5304b2da1f93..94ebfc860040 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -1049,8 +1049,8 @@ int CameraCapabilities::initializeStaticMetadata() > pixelArraySize); > } > > - if (properties.contains(properties::UnitCellSize)) { > - const auto &cellSize = properties.get<Size>(properties::UnitCellSize); > + const auto &cellSize = properties.get<Size>(properties::UnitCellSize); > + if (cellSize) { > std::array<float, 2> physicalSize{ > cellSize->width * pixelArraySize[0] / 1e6f, > cellSize->height * pixelArraySize[1] / 1e6f > @@ -1079,11 +1079,10 @@ int CameraCapabilities::initializeStaticMetadata() > sensitivityRange); > > /* Report the color filter arrangement if the camera reports it. */ > - if (properties.contains(properties::draft::ColorFilterArrangement)) { > - uint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement); > + const auto &filterArr = properties.get(properties::draft::ColorFilterArrangement); > + if (filterArr) > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT, > - filterArr); > - } > + *filterArr); > > const auto &exposureInfo = controlsInfo.find(&controls::ExposureTime); > if (exposureInfo != controlsInfo.end()) { > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 4662123210c2..b20e389b9d30 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -305,9 +305,9 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > */ > const ControlList &properties = camera_->properties(); > > - if (properties.contains(properties::Location)) { > - int32_t location = *properties.get(properties::Location); > - switch (location) { > + const auto &location = properties.get(properties::Location); > + if (location) { > + switch (*location) { > case properties::CameraLocationFront: > facing_ = CAMERA_FACING_FRONT; > break; > @@ -355,9 +355,9 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > * value for clockwise direction as required by the Android orientation > * metadata. > */ > - if (properties.contains(properties::Rotation)) { > - int rotation = *properties.get(properties::Rotation); > - orientation_ = (360 - rotation) % 360; > + const auto &rotation = properties.get(properties::Rotation); > + if (rotation) { > + orientation_ = (360 - *rotation) % 360; > if (cameraConfigData && cameraConfigData->rotation != -1 && > orientation_ != cameraConfigData->rotation) { > LOG(HAL, Warning) > @@ -1564,25 +1564,24 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons > const int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0); > resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp); > > - if (metadata.contains(controls::draft::PipelineDepth)) { > - uint8_t pipeline_depth = *metadata.get<int32_t>(controls::draft::PipelineDepth); > + const auto &pipelineDepth = metadata.get(controls::draft::PipelineDepth); > + if (pipelineDepth) > resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH, > - pipeline_depth); > - } > + *pipelineDepth); > > - if (metadata.contains(controls::ExposureTime)) { > - int64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL; > - resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure); > - } > + const auto &exposureTime = metadata.get(controls::ExposureTime); > + if (exposureTime) > + resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, > + *exposureTime * 1000ULL); > > - if (metadata.contains(controls::FrameDuration)) { > - int64_t duration = *metadata.get(controls::FrameDuration) * 1000; > + const auto &frameDuration = metadata.get(controls::FrameDuration); > + if (frameDuration) > resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, > - duration); > - } > + *frameDuration * 1000); > > - if (metadata.contains(controls::ScalerCrop)) { > - Rectangle crop = *metadata.get(controls::ScalerCrop); > + const auto &scalerCrop = metadata.get(controls::ScalerCrop); > + if (scalerCrop) { > + const Rectangle &crop = *scalerCrop; > int32_t cropRect[] = { > crop.x, crop.y, static_cast<int32_t>(crop.width), > static_cast<int32_t>(crop.height), > @@ -1590,11 +1589,10 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons > resultMetadata->addEntry(ANDROID_SCALER_CROP_REGION, cropRect); > } > > - if (metadata.contains(controls::draft::TestPatternMode)) { > - const int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode); > + const auto &testPatternMode = metadata.get(controls::draft::TestPatternMode); > + if (testPatternMode) > resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, > - testPatternMode); > - } > + *testPatternMode); > > /* > * Return the result metadata pack even is not valid: get() will return > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > index 0bffe96f6dbc..7512cc4e11a4 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -228,11 +228,7 @@ void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam) > > int32_t CameraHalManager::cameraLocation(const Camera *cam) > { > - const ControlList &properties = cam->properties(); > - if (!properties.contains(properties::Location)) > - return -1; > - > - return *properties.get(properties::Location); > + return cam->properties().get(properties::Location).value_or(-1); > } > > CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id) > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index d8115cd86825..13a3d9b93df8 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -300,8 +300,9 @@ std::string CamApp::cameraName(const Camera *camera) > * Construct the name from the camera location, model and ID. The model > * is only used if the location isn't present or is set to External. > */ > - if (props.contains(properties::Location)) { > - switch (*props.get(properties::Location)) { > + const auto &location = props.get(properties::Location); > + if (location) { > + switch (*location) { > case properties::CameraLocationFront: > addModel = false; > name = "Internal front camera "; > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 43db7b68dac8..d60f20b08e27 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1143,18 +1143,17 @@ int PipelineHandlerIPU3::registerCameras() > &IPU3CameraData::frameStart); > > /* Convert the sensor rotation to a transformation */ > - int32_t rotation = 0; > - if (data->properties_.contains(properties::Rotation)) > - rotation = *(data->properties_.get(properties::Rotation)); > - else > + const auto &rotation = data->properties_.get(properties::Rotation); > + if (!rotation) > LOG(IPU3, Warning) << "Rotation control not exposed by " > << cio2->sensor()->id() > << ". Assume rotation 0"; > > + int32_t rotationValue = rotation.value_or(0); > bool success; > - data->rotationTransform_ = transformFromRotation(rotation, &success); > + data->rotationTransform_ = transformFromRotation(rotationValue, &success); > if (!success) > - LOG(IPU3, Warning) << "Invalid rotation of " << rotation > + LOG(IPU3, Warning) << "Invalid rotation of " << rotationValue > << " degrees: ignoring"; > > ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP }); > @@ -1330,8 +1329,9 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > request->metadata().set(controls::draft::PipelineDepth, 3); > /* \todo Actually apply the scaler crop region to the ImgU. */ > - if (request->controls().contains(controls::ScalerCrop)) > - cropRegion_ = *(request->controls().get(controls::ScalerCrop)); > + const auto &scalerCrop = request->controls().get(controls::ScalerCrop); > + if (scalerCrop) > + cropRegion_ = *scalerCrop; > request->metadata().set(controls::ScalerCrop, cropRegion_); > > if (frameInfos_.tryComplete(info)) > @@ -1455,13 +1455,12 @@ void IPU3CameraData::frameStart(uint32_t sequence) > Request *request = processingRequests_.front(); > processingRequests_.pop(); > > - if (!request->controls().contains(controls::draft::TestPatternMode)) > + const auto &testPatternMode = request->controls().get(controls::draft::TestPatternMode); > + if (!testPatternMode) > return; > > - const int32_t testPatternMode = *(request->controls().get(controls::draft::TestPatternMode)); > - > int ret = cio2_.sensor()->setTestPatternMode( > - static_cast<controls::draft::TestPatternModeEnum>(testPatternMode)); > + static_cast<controls::draft::TestPatternModeEnum>(*testPatternMode)); > if (ret) { > LOG(IPU3, Error) << "Failed to set test pattern mode: " > << ret; > @@ -1469,7 +1468,7 @@ void IPU3CameraData::frameStart(uint32_t sequence) > } > > request->metadata().set(controls::draft::TestPatternMode, > - testPatternMode); > + *testPatternMode); > } > > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3) > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 28f054cb84c8..2e19531eecc4 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1716,16 +1716,15 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList & > * Inform the sensor of the latest colour gains if it has the > * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set). > */ > - if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) { > - libcamera::Span<const float> colourGains = > - *controls.get(libcamera::controls::ColourGains); > + const auto &colourGains = controls.get(libcamera::controls::ColourGains); > + if (notifyGainsUnity_ && colourGains) { > /* The control wants linear gains in the order B, Gb, Gr, R. */ > ControlList ctrls(sensor_->controls()); > std::array<int32_t, 4> gains{ > - static_cast<int32_t>(colourGains[1] * *notifyGainsUnity_), > + static_cast<int32_t>((*colourGains)[1] * *notifyGainsUnity_), > *notifyGainsUnity_, > *notifyGainsUnity_, > - static_cast<int32_t>(colourGains[0] * *notifyGainsUnity_) > + static_cast<int32_t>((*colourGains)[0] * *notifyGainsUnity_) > }; > ctrls.set(V4L2_CID_NOTIFY_GAINS, Span<const int32_t>{ gains }); > > @@ -2052,8 +2051,9 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const > > void RPiCameraData::applyScalerCrop(const ControlList &controls) > { > - if (controls.contains(controls::ScalerCrop)) { > - Rectangle nativeCrop = *controls.get<Rectangle>(controls::ScalerCrop); > + const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop); > + if (scalerCrop) { > + Rectangle nativeCrop = *scalerCrop; > > if (!nativeCrop.width || !nativeCrop.height) > nativeCrop = { 0, 0, 1, 1 }; > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp > index 4b5d8276564f..5896485cda5b 100644 > --- a/src/qcam/dng_writer.cpp > +++ b/src/qcam/dng_writer.cpp > @@ -391,9 +391,9 @@ int DNGWriter::write(const char *filename, const Camera *camera, > TIFFSetField(tif, TIFFTAG_FILLORDER, FILLORDER_MSB2LSB); > TIFFSetField(tif, TIFFTAG_MAKE, "libcamera"); > > - if (cameraProperties.contains(properties::Model)) { > - std::string model = *cameraProperties.get(properties::Model); > - TIFFSetField(tif, TIFFTAG_MODEL, model.c_str()); > + const auto &model = cameraProperties.get(properties::Model); > + if (model) { > + TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); > /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ > } > > @@ -437,16 +437,18 @@ int DNGWriter::write(const char *filename, const Camera *camera, > */ > const double eps = 1e-2; > > - if (metadata.contains(controls::ColourGains)) { > - const auto &colourGains = metadata.get(controls::ColourGains); > + const auto &colourGains = metadata.get(controls::ColourGains); > + if (colourGains) { > if ((*colourGains)[0] > eps && (*colourGains)[1] > eps) { > wbGain = Matrix3d::diag((*colourGains)[0], 1, (*colourGains)[1]); > neutral[0] = 1.0 / (*colourGains)[0]; /* red */ > neutral[2] = 1.0 / (*colourGains)[1]; /* blue */ > } > } > - if (metadata.contains(controls::ColourCorrectionMatrix)) { > - Matrix3d ccmSupplied(*metadata.get(controls::ColourCorrectionMatrix)); > + > + const auto &ccmControl = metadata.get(controls::ColourCorrectionMatrix); > + if (ccmControl) { > + Matrix3d ccmSupplied(*ccmControl); > if (ccmSupplied.determinant() > eps) > ccm = ccmSupplied; > } > @@ -513,9 +515,9 @@ int DNGWriter::write(const char *filename, const Camera *camera, > float blackLevel[] = { 0.0f, 0.0f, 0.0f, 0.0f }; > uint32_t whiteLevel = (1 << info->bitsPerSample) - 1; > > - if (metadata.contains(controls::SensorBlackLevels)) { > - Span<const int32_t> levels = > - *metadata.get(controls::SensorBlackLevels); > + const auto &blackLevels = metadata.get(controls::SensorBlackLevels); > + if (blackLevels) { > + Span<const int32_t> levels = *blackLevels; > > /* > * The black levels control is specified in R, Gr, Gb, B order. > @@ -592,16 +594,15 @@ int DNGWriter::write(const char *filename, const Camera *camera, > TIFFSetField(tif, EXIFTAG_DATETIMEORIGINAL, strTime); > TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime); > > - if (metadata.contains(controls::AnalogueGain)) { > - float gain = *metadata.get(controls::AnalogueGain); > - uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f); > + const auto &analogGain = metadata.get(controls::AnalogueGain); > + if (analogGain) { > + uint16_t iso = std::min(std::max(*analogGain * 100, 0.0f), 65535.0f); > TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso); > } > > - if (metadata.contains(controls::ExposureTime)) { > - float exposureTime = *metadata.get(controls::ExposureTime) / 1e6; > - TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime); > - } > + const auto &exposureTime = metadata.get(controls::ExposureTime); > + if (exposureTime) > + TIFFSetField(tif, EXIFTAG_EXPOSURETIME, *exposureTime / 1e6); > > TIFFWriteCustomDirectory(tif, &exifIFDOffset); >
HI Laurent, On Mon, Jul 11, 2022 at 03:39:18AM +0300, Laurent Pinchart via libcamera-devel wrote: > Now that the ControlList::get() function returns an instance of > std::optional<>, we can replace the ControlList::contains() calls with a > nullopt check on the return value of get(). This avoids double lookups > of controls through the code base. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Looks good thank you! Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > This patch applies on top of "[PATCH v10 1/2] libcamera: controls: Use > std::optional to handle invalid control values". > --- > src/android/camera_capabilities.cpp | 11 ++--- > src/android/camera_device.cpp | 46 +++++++++---------- > src/android/camera_hal_manager.cpp | 6 +-- > src/cam/main.cpp | 5 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 25 +++++----- > .../pipeline/raspberrypi/raspberrypi.cpp | 14 +++--- > src/qcam/dng_writer.cpp | 35 +++++++------- > 7 files changed, 68 insertions(+), 74 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 5304b2da1f93..94ebfc860040 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -1049,8 +1049,8 @@ int CameraCapabilities::initializeStaticMetadata() > pixelArraySize); > } > > - if (properties.contains(properties::UnitCellSize)) { > - const auto &cellSize = properties.get<Size>(properties::UnitCellSize); > + const auto &cellSize = properties.get<Size>(properties::UnitCellSize); > + if (cellSize) { > std::array<float, 2> physicalSize{ > cellSize->width * pixelArraySize[0] / 1e6f, > cellSize->height * pixelArraySize[1] / 1e6f > @@ -1079,11 +1079,10 @@ int CameraCapabilities::initializeStaticMetadata() > sensitivityRange); > > /* Report the color filter arrangement if the camera reports it. */ > - if (properties.contains(properties::draft::ColorFilterArrangement)) { > - uint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement); > + const auto &filterArr = properties.get(properties::draft::ColorFilterArrangement); > + if (filterArr) > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT, > - filterArr); > - } > + *filterArr); > > const auto &exposureInfo = controlsInfo.find(&controls::ExposureTime); > if (exposureInfo != controlsInfo.end()) { > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 4662123210c2..b20e389b9d30 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -305,9 +305,9 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > */ > const ControlList &properties = camera_->properties(); > > - if (properties.contains(properties::Location)) { > - int32_t location = *properties.get(properties::Location); > - switch (location) { > + const auto &location = properties.get(properties::Location); > + if (location) { > + switch (*location) { > case properties::CameraLocationFront: > facing_ = CAMERA_FACING_FRONT; > break; > @@ -355,9 +355,9 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > * value for clockwise direction as required by the Android orientation > * metadata. > */ > - if (properties.contains(properties::Rotation)) { > - int rotation = *properties.get(properties::Rotation); > - orientation_ = (360 - rotation) % 360; > + const auto &rotation = properties.get(properties::Rotation); > + if (rotation) { > + orientation_ = (360 - *rotation) % 360; > if (cameraConfigData && cameraConfigData->rotation != -1 && > orientation_ != cameraConfigData->rotation) { > LOG(HAL, Warning) > @@ -1564,25 +1564,24 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons > const int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0); > resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp); > > - if (metadata.contains(controls::draft::PipelineDepth)) { > - uint8_t pipeline_depth = *metadata.get<int32_t>(controls::draft::PipelineDepth); > + const auto &pipelineDepth = metadata.get(controls::draft::PipelineDepth); > + if (pipelineDepth) > resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH, > - pipeline_depth); > - } > + *pipelineDepth); > > - if (metadata.contains(controls::ExposureTime)) { > - int64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL; > - resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure); > - } > + const auto &exposureTime = metadata.get(controls::ExposureTime); > + if (exposureTime) > + resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, > + *exposureTime * 1000ULL); > > - if (metadata.contains(controls::FrameDuration)) { > - int64_t duration = *metadata.get(controls::FrameDuration) * 1000; > + const auto &frameDuration = metadata.get(controls::FrameDuration); > + if (frameDuration) > resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, > - duration); > - } > + *frameDuration * 1000); > > - if (metadata.contains(controls::ScalerCrop)) { > - Rectangle crop = *metadata.get(controls::ScalerCrop); > + const auto &scalerCrop = metadata.get(controls::ScalerCrop); > + if (scalerCrop) { > + const Rectangle &crop = *scalerCrop; > int32_t cropRect[] = { > crop.x, crop.y, static_cast<int32_t>(crop.width), > static_cast<int32_t>(crop.height), > @@ -1590,11 +1589,10 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons > resultMetadata->addEntry(ANDROID_SCALER_CROP_REGION, cropRect); > } > > - if (metadata.contains(controls::draft::TestPatternMode)) { > - const int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode); > + const auto &testPatternMode = metadata.get(controls::draft::TestPatternMode); > + if (testPatternMode) > resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, > - testPatternMode); > - } > + *testPatternMode); > > /* > * Return the result metadata pack even is not valid: get() will return > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > index 0bffe96f6dbc..7512cc4e11a4 100644 > --- a/src/android/camera_hal_manager.cpp > +++ b/src/android/camera_hal_manager.cpp > @@ -228,11 +228,7 @@ void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam) > > int32_t CameraHalManager::cameraLocation(const Camera *cam) > { > - const ControlList &properties = cam->properties(); > - if (!properties.contains(properties::Location)) > - return -1; > - > - return *properties.get(properties::Location); > + return cam->properties().get(properties::Location).value_or(-1); > } > > CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id) > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > index d8115cd86825..13a3d9b93df8 100644 > --- a/src/cam/main.cpp > +++ b/src/cam/main.cpp > @@ -300,8 +300,9 @@ std::string CamApp::cameraName(const Camera *camera) > * Construct the name from the camera location, model and ID. The model > * is only used if the location isn't present or is set to External. > */ > - if (props.contains(properties::Location)) { > - switch (*props.get(properties::Location)) { > + const auto &location = props.get(properties::Location); > + if (location) { > + switch (*location) { > case properties::CameraLocationFront: > addModel = false; > name = "Internal front camera "; > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 43db7b68dac8..d60f20b08e27 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1143,18 +1143,17 @@ int PipelineHandlerIPU3::registerCameras() > &IPU3CameraData::frameStart); > > /* Convert the sensor rotation to a transformation */ > - int32_t rotation = 0; > - if (data->properties_.contains(properties::Rotation)) > - rotation = *(data->properties_.get(properties::Rotation)); > - else > + const auto &rotation = data->properties_.get(properties::Rotation); > + if (!rotation) > LOG(IPU3, Warning) << "Rotation control not exposed by " > << cio2->sensor()->id() > << ". Assume rotation 0"; > > + int32_t rotationValue = rotation.value_or(0); > bool success; > - data->rotationTransform_ = transformFromRotation(rotation, &success); > + data->rotationTransform_ = transformFromRotation(rotationValue, &success); > if (!success) > - LOG(IPU3, Warning) << "Invalid rotation of " << rotation > + LOG(IPU3, Warning) << "Invalid rotation of " << rotationValue > << " degrees: ignoring"; > > ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP }); > @@ -1330,8 +1329,9 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > request->metadata().set(controls::draft::PipelineDepth, 3); > /* \todo Actually apply the scaler crop region to the ImgU. */ > - if (request->controls().contains(controls::ScalerCrop)) > - cropRegion_ = *(request->controls().get(controls::ScalerCrop)); > + const auto &scalerCrop = request->controls().get(controls::ScalerCrop); > + if (scalerCrop) > + cropRegion_ = *scalerCrop; > request->metadata().set(controls::ScalerCrop, cropRegion_); > > if (frameInfos_.tryComplete(info)) > @@ -1455,13 +1455,12 @@ void IPU3CameraData::frameStart(uint32_t sequence) > Request *request = processingRequests_.front(); > processingRequests_.pop(); > > - if (!request->controls().contains(controls::draft::TestPatternMode)) > + const auto &testPatternMode = request->controls().get(controls::draft::TestPatternMode); > + if (!testPatternMode) > return; > > - const int32_t testPatternMode = *(request->controls().get(controls::draft::TestPatternMode)); > - > int ret = cio2_.sensor()->setTestPatternMode( > - static_cast<controls::draft::TestPatternModeEnum>(testPatternMode)); > + static_cast<controls::draft::TestPatternModeEnum>(*testPatternMode)); > if (ret) { > LOG(IPU3, Error) << "Failed to set test pattern mode: " > << ret; > @@ -1469,7 +1468,7 @@ void IPU3CameraData::frameStart(uint32_t sequence) > } > > request->metadata().set(controls::draft::TestPatternMode, > - testPatternMode); > + *testPatternMode); > } > > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3) > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 28f054cb84c8..2e19531eecc4 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1716,16 +1716,15 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList & > * Inform the sensor of the latest colour gains if it has the > * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set). > */ > - if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) { > - libcamera::Span<const float> colourGains = > - *controls.get(libcamera::controls::ColourGains); > + const auto &colourGains = controls.get(libcamera::controls::ColourGains); > + if (notifyGainsUnity_ && colourGains) { > /* The control wants linear gains in the order B, Gb, Gr, R. */ > ControlList ctrls(sensor_->controls()); > std::array<int32_t, 4> gains{ > - static_cast<int32_t>(colourGains[1] * *notifyGainsUnity_), > + static_cast<int32_t>((*colourGains)[1] * *notifyGainsUnity_), > *notifyGainsUnity_, > *notifyGainsUnity_, > - static_cast<int32_t>(colourGains[0] * *notifyGainsUnity_) > + static_cast<int32_t>((*colourGains)[0] * *notifyGainsUnity_) > }; > ctrls.set(V4L2_CID_NOTIFY_GAINS, Span<const int32_t>{ gains }); > > @@ -2052,8 +2051,9 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const > > void RPiCameraData::applyScalerCrop(const ControlList &controls) > { > - if (controls.contains(controls::ScalerCrop)) { > - Rectangle nativeCrop = *controls.get<Rectangle>(controls::ScalerCrop); > + const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop); > + if (scalerCrop) { > + Rectangle nativeCrop = *scalerCrop; > > if (!nativeCrop.width || !nativeCrop.height) > nativeCrop = { 0, 0, 1, 1 }; > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp > index 4b5d8276564f..5896485cda5b 100644 > --- a/src/qcam/dng_writer.cpp > +++ b/src/qcam/dng_writer.cpp > @@ -391,9 +391,9 @@ int DNGWriter::write(const char *filename, const Camera *camera, > TIFFSetField(tif, TIFFTAG_FILLORDER, FILLORDER_MSB2LSB); > TIFFSetField(tif, TIFFTAG_MAKE, "libcamera"); > > - if (cameraProperties.contains(properties::Model)) { > - std::string model = *cameraProperties.get(properties::Model); > - TIFFSetField(tif, TIFFTAG_MODEL, model.c_str()); > + const auto &model = cameraProperties.get(properties::Model); > + if (model) { > + TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); > /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ > } > > @@ -437,16 +437,18 @@ int DNGWriter::write(const char *filename, const Camera *camera, > */ > const double eps = 1e-2; > > - if (metadata.contains(controls::ColourGains)) { > - const auto &colourGains = metadata.get(controls::ColourGains); > + const auto &colourGains = metadata.get(controls::ColourGains); > + if (colourGains) { > if ((*colourGains)[0] > eps && (*colourGains)[1] > eps) { > wbGain = Matrix3d::diag((*colourGains)[0], 1, (*colourGains)[1]); > neutral[0] = 1.0 / (*colourGains)[0]; /* red */ > neutral[2] = 1.0 / (*colourGains)[1]; /* blue */ > } > } > - if (metadata.contains(controls::ColourCorrectionMatrix)) { > - Matrix3d ccmSupplied(*metadata.get(controls::ColourCorrectionMatrix)); > + > + const auto &ccmControl = metadata.get(controls::ColourCorrectionMatrix); > + if (ccmControl) { > + Matrix3d ccmSupplied(*ccmControl); > if (ccmSupplied.determinant() > eps) > ccm = ccmSupplied; > } > @@ -513,9 +515,9 @@ int DNGWriter::write(const char *filename, const Camera *camera, > float blackLevel[] = { 0.0f, 0.0f, 0.0f, 0.0f }; > uint32_t whiteLevel = (1 << info->bitsPerSample) - 1; > > - if (metadata.contains(controls::SensorBlackLevels)) { > - Span<const int32_t> levels = > - *metadata.get(controls::SensorBlackLevels); > + const auto &blackLevels = metadata.get(controls::SensorBlackLevels); > + if (blackLevels) { > + Span<const int32_t> levels = *blackLevels; > > /* > * The black levels control is specified in R, Gr, Gb, B order. > @@ -592,16 +594,15 @@ int DNGWriter::write(const char *filename, const Camera *camera, > TIFFSetField(tif, EXIFTAG_DATETIMEORIGINAL, strTime); > TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime); > > - if (metadata.contains(controls::AnalogueGain)) { > - float gain = *metadata.get(controls::AnalogueGain); > - uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f); > + const auto &analogGain = metadata.get(controls::AnalogueGain); > + if (analogGain) { > + uint16_t iso = std::min(std::max(*analogGain * 100, 0.0f), 65535.0f); > TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso); > } > > - if (metadata.contains(controls::ExposureTime)) { > - float exposureTime = *metadata.get(controls::ExposureTime) / 1e6; > - TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime); > - } > + const auto &exposureTime = metadata.get(controls::ExposureTime); > + if (exposureTime) > + TIFFSetField(tif, EXIFTAG_EXPOSURETIME, *exposureTime / 1e6); > > TIFFWriteCustomDirectory(tif, &exifIFDOffset); > > -- > Regards, > > Laurent Pinchart >
Hi Umang, On Thu, Jul 14, 2022 at 01:08:41PM +0530, Umang Jain wrote: > On 7/11/22 06:09, Laurent Pinchart via libcamera-devel wrote: > > Now that the ControlList::get() function returns an instance of > > std::optional<>, we can replace the ControlList::contains() calls with a > > nullopt check on the return value of get(). This avoids double lookups > > of controls through the code base. > > The patch looks good to me, but I had a follow-up question whether the > change of ControlList::get() returning a std::optional<> means we can > drop .contains() helper probably? What do you think? Eventually yes, I think. There's a "little" issue I'd like to solve first though, if possible. ControlList::get() returns a std::optional<T>, which means it copies the value from the internal ControlValue storage to std::optional<T>. In most cases that's fine, but when we'll have large controls (large arrays for instance) that's not optimal. There would be a copy just to check if a control exists, which isn't great. I haven't found a solution I like yet. We can't use a std::optional<T&> as the type needs to be assignable (you can't change a reference after it's set), and std::optional<std::reference_wrapper<T>> makes the syntax more cumbersome in callers. This being said, there should be very few cases where we're interested in knowing if a control exists without needing his value right after, so maybe we could already drop the functions. I actually gave it a quick try, and it looks like contains(const ControlId &id) is used in two places only: - src/cam/main.cpp, which I didn't notice when writing this patch, so I'll send a v2 - test/controls/control_list.cpp, which we could change fairly easily I'll give it a go on top of this patch. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > This patch applies on top of "[PATCH v10 1/2] libcamera: controls: Use > > std::optional to handle invalid control values". > > --- > > src/android/camera_capabilities.cpp | 11 ++--- > > src/android/camera_device.cpp | 46 +++++++++---------- > > src/android/camera_hal_manager.cpp | 6 +-- > > src/cam/main.cpp | 5 +- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 25 +++++----- > > .../pipeline/raspberrypi/raspberrypi.cpp | 14 +++--- > > src/qcam/dng_writer.cpp | 35 +++++++------- > > 7 files changed, 68 insertions(+), 74 deletions(-) > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > index 5304b2da1f93..94ebfc860040 100644 > > --- a/src/android/camera_capabilities.cpp > > +++ b/src/android/camera_capabilities.cpp > > @@ -1049,8 +1049,8 @@ int CameraCapabilities::initializeStaticMetadata() > > pixelArraySize); > > } > > > > - if (properties.contains(properties::UnitCellSize)) { > > - const auto &cellSize = properties.get<Size>(properties::UnitCellSize); > > + const auto &cellSize = properties.get<Size>(properties::UnitCellSize); > > + if (cellSize) { > > std::array<float, 2> physicalSize{ > > cellSize->width * pixelArraySize[0] / 1e6f, > > cellSize->height * pixelArraySize[1] / 1e6f > > @@ -1079,11 +1079,10 @@ int CameraCapabilities::initializeStaticMetadata() > > sensitivityRange); > > > > /* Report the color filter arrangement if the camera reports it. */ > > - if (properties.contains(properties::draft::ColorFilterArrangement)) { > > - uint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement); > > + const auto &filterArr = properties.get(properties::draft::ColorFilterArrangement); > > + if (filterArr) > > staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT, > > - filterArr); > > - } > > + *filterArr); > > > > const auto &exposureInfo = controlsInfo.find(&controls::ExposureTime); > > if (exposureInfo != controlsInfo.end()) { > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 4662123210c2..b20e389b9d30 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -305,9 +305,9 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > > */ > > const ControlList &properties = camera_->properties(); > > > > - if (properties.contains(properties::Location)) { > > - int32_t location = *properties.get(properties::Location); > > - switch (location) { > > + const auto &location = properties.get(properties::Location); > > + if (location) { > > + switch (*location) { > > case properties::CameraLocationFront: > > facing_ = CAMERA_FACING_FRONT; > > break; > > @@ -355,9 +355,9 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) > > * value for clockwise direction as required by the Android orientation > > * metadata. > > */ > > - if (properties.contains(properties::Rotation)) { > > - int rotation = *properties.get(properties::Rotation); > > - orientation_ = (360 - rotation) % 360; > > + const auto &rotation = properties.get(properties::Rotation); > > + if (rotation) { > > + orientation_ = (360 - *rotation) % 360; > > if (cameraConfigData && cameraConfigData->rotation != -1 && > > orientation_ != cameraConfigData->rotation) { > > LOG(HAL, Warning) > > @@ -1564,25 +1564,24 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons > > const int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0); > > resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp); > > > > - if (metadata.contains(controls::draft::PipelineDepth)) { > > - uint8_t pipeline_depth = *metadata.get<int32_t>(controls::draft::PipelineDepth); > > + const auto &pipelineDepth = metadata.get(controls::draft::PipelineDepth); > > + if (pipelineDepth) > > resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH, > > - pipeline_depth); > > - } > > + *pipelineDepth); > > > > - if (metadata.contains(controls::ExposureTime)) { > > - int64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL; > > - resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure); > > - } > > + const auto &exposureTime = metadata.get(controls::ExposureTime); > > + if (exposureTime) > > + resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, > > + *exposureTime * 1000ULL); > > > > - if (metadata.contains(controls::FrameDuration)) { > > - int64_t duration = *metadata.get(controls::FrameDuration) * 1000; > > + const auto &frameDuration = metadata.get(controls::FrameDuration); > > + if (frameDuration) > > resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, > > - duration); > > - } > > + *frameDuration * 1000); > > > > - if (metadata.contains(controls::ScalerCrop)) { > > - Rectangle crop = *metadata.get(controls::ScalerCrop); > > + const auto &scalerCrop = metadata.get(controls::ScalerCrop); > > + if (scalerCrop) { > > + const Rectangle &crop = *scalerCrop; > > int32_t cropRect[] = { > > crop.x, crop.y, static_cast<int32_t>(crop.width), > > static_cast<int32_t>(crop.height), > > @@ -1590,11 +1589,10 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons > > resultMetadata->addEntry(ANDROID_SCALER_CROP_REGION, cropRect); > > } > > > > - if (metadata.contains(controls::draft::TestPatternMode)) { > > - const int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode); > > + const auto &testPatternMode = metadata.get(controls::draft::TestPatternMode); > > + if (testPatternMode) > > resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, > > - testPatternMode); > > - } > > + *testPatternMode); > > > > /* > > * Return the result metadata pack even is not valid: get() will return > > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp > > index 0bffe96f6dbc..7512cc4e11a4 100644 > > --- a/src/android/camera_hal_manager.cpp > > +++ b/src/android/camera_hal_manager.cpp > > @@ -228,11 +228,7 @@ void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam) > > > > int32_t CameraHalManager::cameraLocation(const Camera *cam) > > { > > - const ControlList &properties = cam->properties(); > > - if (!properties.contains(properties::Location)) > > - return -1; > > - > > - return *properties.get(properties::Location); > > + return cam->properties().get(properties::Location).value_or(-1); > > } > > > > CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id) > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp > > index d8115cd86825..13a3d9b93df8 100644 > > --- a/src/cam/main.cpp > > +++ b/src/cam/main.cpp > > @@ -300,8 +300,9 @@ std::string CamApp::cameraName(const Camera *camera) > > * Construct the name from the camera location, model and ID. The model > > * is only used if the location isn't present or is set to External. > > */ > > - if (props.contains(properties::Location)) { > > - switch (*props.get(properties::Location)) { > > + const auto &location = props.get(properties::Location); > > + if (location) { > > + switch (*location) { > > case properties::CameraLocationFront: > > addModel = false; > > name = "Internal front camera "; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 43db7b68dac8..d60f20b08e27 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -1143,18 +1143,17 @@ int PipelineHandlerIPU3::registerCameras() > > &IPU3CameraData::frameStart); > > > > /* Convert the sensor rotation to a transformation */ > > - int32_t rotation = 0; > > - if (data->properties_.contains(properties::Rotation)) > > - rotation = *(data->properties_.get(properties::Rotation)); > > - else > > + const auto &rotation = data->properties_.get(properties::Rotation); > > + if (!rotation) > > LOG(IPU3, Warning) << "Rotation control not exposed by " > > << cio2->sensor()->id() > > << ". Assume rotation 0"; > > > > + int32_t rotationValue = rotation.value_or(0); > > bool success; > > - data->rotationTransform_ = transformFromRotation(rotation, &success); > > + data->rotationTransform_ = transformFromRotation(rotationValue, &success); > > if (!success) > > - LOG(IPU3, Warning) << "Invalid rotation of " << rotation > > + LOG(IPU3, Warning) << "Invalid rotation of " << rotationValue > > << " degrees: ignoring"; > > > > ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP }); > > @@ -1330,8 +1329,9 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > > > request->metadata().set(controls::draft::PipelineDepth, 3); > > /* \todo Actually apply the scaler crop region to the ImgU. */ > > - if (request->controls().contains(controls::ScalerCrop)) > > - cropRegion_ = *(request->controls().get(controls::ScalerCrop)); > > + const auto &scalerCrop = request->controls().get(controls::ScalerCrop); > > + if (scalerCrop) > > + cropRegion_ = *scalerCrop; > > request->metadata().set(controls::ScalerCrop, cropRegion_); > > > > if (frameInfos_.tryComplete(info)) > > @@ -1455,13 +1455,12 @@ void IPU3CameraData::frameStart(uint32_t sequence) > > Request *request = processingRequests_.front(); > > processingRequests_.pop(); > > > > - if (!request->controls().contains(controls::draft::TestPatternMode)) > > + const auto &testPatternMode = request->controls().get(controls::draft::TestPatternMode); > > + if (!testPatternMode) > > return; > > > > - const int32_t testPatternMode = *(request->controls().get(controls::draft::TestPatternMode)); > > - > > int ret = cio2_.sensor()->setTestPatternMode( > > - static_cast<controls::draft::TestPatternModeEnum>(testPatternMode)); > > + static_cast<controls::draft::TestPatternModeEnum>(*testPatternMode)); > > if (ret) { > > LOG(IPU3, Error) << "Failed to set test pattern mode: " > > << ret; > > @@ -1469,7 +1468,7 @@ void IPU3CameraData::frameStart(uint32_t sequence) > > } > > > > request->metadata().set(controls::draft::TestPatternMode, > > - testPatternMode); > > + *testPatternMode); > > } > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 28f054cb84c8..2e19531eecc4 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1716,16 +1716,15 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList & > > * Inform the sensor of the latest colour gains if it has the > > * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set). > > */ > > - if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) { > > - libcamera::Span<const float> colourGains = > > - *controls.get(libcamera::controls::ColourGains); > > + const auto &colourGains = controls.get(libcamera::controls::ColourGains); > > + if (notifyGainsUnity_ && colourGains) { > > /* The control wants linear gains in the order B, Gb, Gr, R. */ > > ControlList ctrls(sensor_->controls()); > > std::array<int32_t, 4> gains{ > > - static_cast<int32_t>(colourGains[1] * *notifyGainsUnity_), > > + static_cast<int32_t>((*colourGains)[1] * *notifyGainsUnity_), > > *notifyGainsUnity_, > > *notifyGainsUnity_, > > - static_cast<int32_t>(colourGains[0] * *notifyGainsUnity_) > > + static_cast<int32_t>((*colourGains)[0] * *notifyGainsUnity_) > > }; > > ctrls.set(V4L2_CID_NOTIFY_GAINS, Span<const int32_t>{ gains }); > > > > @@ -2052,8 +2051,9 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const > > > > void RPiCameraData::applyScalerCrop(const ControlList &controls) > > { > > - if (controls.contains(controls::ScalerCrop)) { > > - Rectangle nativeCrop = *controls.get<Rectangle>(controls::ScalerCrop); > > + const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop); > > + if (scalerCrop) { > > + Rectangle nativeCrop = *scalerCrop; > > > > if (!nativeCrop.width || !nativeCrop.height) > > nativeCrop = { 0, 0, 1, 1 }; > > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp > > index 4b5d8276564f..5896485cda5b 100644 > > --- a/src/qcam/dng_writer.cpp > > +++ b/src/qcam/dng_writer.cpp > > @@ -391,9 +391,9 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > TIFFSetField(tif, TIFFTAG_FILLORDER, FILLORDER_MSB2LSB); > > TIFFSetField(tif, TIFFTAG_MAKE, "libcamera"); > > > > - if (cameraProperties.contains(properties::Model)) { > > - std::string model = *cameraProperties.get(properties::Model); > > - TIFFSetField(tif, TIFFTAG_MODEL, model.c_str()); > > + const auto &model = cameraProperties.get(properties::Model); > > + if (model) { > > + TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); > > /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ > > } > > > > @@ -437,16 +437,18 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > */ > > const double eps = 1e-2; > > > > - if (metadata.contains(controls::ColourGains)) { > > - const auto &colourGains = metadata.get(controls::ColourGains); > > + const auto &colourGains = metadata.get(controls::ColourGains); > > + if (colourGains) { > > if ((*colourGains)[0] > eps && (*colourGains)[1] > eps) { > > wbGain = Matrix3d::diag((*colourGains)[0], 1, (*colourGains)[1]); > > neutral[0] = 1.0 / (*colourGains)[0]; /* red */ > > neutral[2] = 1.0 / (*colourGains)[1]; /* blue */ > > } > > } > > - if (metadata.contains(controls::ColourCorrectionMatrix)) { > > - Matrix3d ccmSupplied(*metadata.get(controls::ColourCorrectionMatrix)); > > + > > + const auto &ccmControl = metadata.get(controls::ColourCorrectionMatrix); > > + if (ccmControl) { > > + Matrix3d ccmSupplied(*ccmControl); > > if (ccmSupplied.determinant() > eps) > > ccm = ccmSupplied; > > } > > @@ -513,9 +515,9 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > float blackLevel[] = { 0.0f, 0.0f, 0.0f, 0.0f }; > > uint32_t whiteLevel = (1 << info->bitsPerSample) - 1; > > > > - if (metadata.contains(controls::SensorBlackLevels)) { > > - Span<const int32_t> levels = > > - *metadata.get(controls::SensorBlackLevels); > > + const auto &blackLevels = metadata.get(controls::SensorBlackLevels); > > + if (blackLevels) { > > + Span<const int32_t> levels = *blackLevels; > > > > /* > > * The black levels control is specified in R, Gr, Gb, B order. > > @@ -592,16 +594,15 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > TIFFSetField(tif, EXIFTAG_DATETIMEORIGINAL, strTime); > > TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime); > > > > - if (metadata.contains(controls::AnalogueGain)) { > > - float gain = *metadata.get(controls::AnalogueGain); > > - uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f); > > + const auto &analogGain = metadata.get(controls::AnalogueGain); > > + if (analogGain) { > > + uint16_t iso = std::min(std::max(*analogGain * 100, 0.0f), 65535.0f); > > TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso); > > } > > > > - if (metadata.contains(controls::ExposureTime)) { > > - float exposureTime = *metadata.get(controls::ExposureTime) / 1e6; > > - TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime); > > - } > > + const auto &exposureTime = metadata.get(controls::ExposureTime); > > + if (exposureTime) > > + TIFFSetField(tif, EXIFTAG_EXPOSURETIME, *exposureTime / 1e6); > > > > TIFFWriteCustomDirectory(tif, &exifIFDOffset); > >
diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp index 5304b2da1f93..94ebfc860040 100644 --- a/src/android/camera_capabilities.cpp +++ b/src/android/camera_capabilities.cpp @@ -1049,8 +1049,8 @@ int CameraCapabilities::initializeStaticMetadata() pixelArraySize); } - if (properties.contains(properties::UnitCellSize)) { - const auto &cellSize = properties.get<Size>(properties::UnitCellSize); + const auto &cellSize = properties.get<Size>(properties::UnitCellSize); + if (cellSize) { std::array<float, 2> physicalSize{ cellSize->width * pixelArraySize[0] / 1e6f, cellSize->height * pixelArraySize[1] / 1e6f @@ -1079,11 +1079,10 @@ int CameraCapabilities::initializeStaticMetadata() sensitivityRange); /* Report the color filter arrangement if the camera reports it. */ - if (properties.contains(properties::draft::ColorFilterArrangement)) { - uint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement); + const auto &filterArr = properties.get(properties::draft::ColorFilterArrangement); + if (filterArr) staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT, - filterArr); - } + *filterArr); const auto &exposureInfo = controlsInfo.find(&controls::ExposureTime); if (exposureInfo != controlsInfo.end()) { diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 4662123210c2..b20e389b9d30 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -305,9 +305,9 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) */ const ControlList &properties = camera_->properties(); - if (properties.contains(properties::Location)) { - int32_t location = *properties.get(properties::Location); - switch (location) { + const auto &location = properties.get(properties::Location); + if (location) { + switch (*location) { case properties::CameraLocationFront: facing_ = CAMERA_FACING_FRONT; break; @@ -355,9 +355,9 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData) * value for clockwise direction as required by the Android orientation * metadata. */ - if (properties.contains(properties::Rotation)) { - int rotation = *properties.get(properties::Rotation); - orientation_ = (360 - rotation) % 360; + const auto &rotation = properties.get(properties::Rotation); + if (rotation) { + orientation_ = (360 - *rotation) % 360; if (cameraConfigData && cameraConfigData->rotation != -1 && orientation_ != cameraConfigData->rotation) { LOG(HAL, Warning) @@ -1564,25 +1564,24 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons const int64_t timestamp = metadata.get(controls::SensorTimestamp).value_or(0); resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, timestamp); - if (metadata.contains(controls::draft::PipelineDepth)) { - uint8_t pipeline_depth = *metadata.get<int32_t>(controls::draft::PipelineDepth); + const auto &pipelineDepth = metadata.get(controls::draft::PipelineDepth); + if (pipelineDepth) resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH, - pipeline_depth); - } + *pipelineDepth); - if (metadata.contains(controls::ExposureTime)) { - int64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL; - resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure); - } + const auto &exposureTime = metadata.get(controls::ExposureTime); + if (exposureTime) + resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, + *exposureTime * 1000ULL); - if (metadata.contains(controls::FrameDuration)) { - int64_t duration = *metadata.get(controls::FrameDuration) * 1000; + const auto &frameDuration = metadata.get(controls::FrameDuration); + if (frameDuration) resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION, - duration); - } + *frameDuration * 1000); - if (metadata.contains(controls::ScalerCrop)) { - Rectangle crop = *metadata.get(controls::ScalerCrop); + const auto &scalerCrop = metadata.get(controls::ScalerCrop); + if (scalerCrop) { + const Rectangle &crop = *scalerCrop; int32_t cropRect[] = { crop.x, crop.y, static_cast<int32_t>(crop.width), static_cast<int32_t>(crop.height), @@ -1590,11 +1589,10 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons resultMetadata->addEntry(ANDROID_SCALER_CROP_REGION, cropRect); } - if (metadata.contains(controls::draft::TestPatternMode)) { - const int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode); + const auto &testPatternMode = metadata.get(controls::draft::TestPatternMode); + if (testPatternMode) resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE, - testPatternMode); - } + *testPatternMode); /* * Return the result metadata pack even is not valid: get() will return diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp index 0bffe96f6dbc..7512cc4e11a4 100644 --- a/src/android/camera_hal_manager.cpp +++ b/src/android/camera_hal_manager.cpp @@ -228,11 +228,7 @@ void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam) int32_t CameraHalManager::cameraLocation(const Camera *cam) { - const ControlList &properties = cam->properties(); - if (!properties.contains(properties::Location)) - return -1; - - return *properties.get(properties::Location); + return cam->properties().get(properties::Location).value_or(-1); } CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id) diff --git a/src/cam/main.cpp b/src/cam/main.cpp index d8115cd86825..13a3d9b93df8 100644 --- a/src/cam/main.cpp +++ b/src/cam/main.cpp @@ -300,8 +300,9 @@ std::string CamApp::cameraName(const Camera *camera) * Construct the name from the camera location, model and ID. The model * is only used if the location isn't present or is set to External. */ - if (props.contains(properties::Location)) { - switch (*props.get(properties::Location)) { + const auto &location = props.get(properties::Location); + if (location) { + switch (*location) { case properties::CameraLocationFront: addModel = false; name = "Internal front camera "; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 43db7b68dac8..d60f20b08e27 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -1143,18 +1143,17 @@ int PipelineHandlerIPU3::registerCameras() &IPU3CameraData::frameStart); /* Convert the sensor rotation to a transformation */ - int32_t rotation = 0; - if (data->properties_.contains(properties::Rotation)) - rotation = *(data->properties_.get(properties::Rotation)); - else + const auto &rotation = data->properties_.get(properties::Rotation); + if (!rotation) LOG(IPU3, Warning) << "Rotation control not exposed by " << cio2->sensor()->id() << ". Assume rotation 0"; + int32_t rotationValue = rotation.value_or(0); bool success; - data->rotationTransform_ = transformFromRotation(rotation, &success); + data->rotationTransform_ = transformFromRotation(rotationValue, &success); if (!success) - LOG(IPU3, Warning) << "Invalid rotation of " << rotation + LOG(IPU3, Warning) << "Invalid rotation of " << rotationValue << " degrees: ignoring"; ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP }); @@ -1330,8 +1329,9 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) request->metadata().set(controls::draft::PipelineDepth, 3); /* \todo Actually apply the scaler crop region to the ImgU. */ - if (request->controls().contains(controls::ScalerCrop)) - cropRegion_ = *(request->controls().get(controls::ScalerCrop)); + const auto &scalerCrop = request->controls().get(controls::ScalerCrop); + if (scalerCrop) + cropRegion_ = *scalerCrop; request->metadata().set(controls::ScalerCrop, cropRegion_); if (frameInfos_.tryComplete(info)) @@ -1455,13 +1455,12 @@ void IPU3CameraData::frameStart(uint32_t sequence) Request *request = processingRequests_.front(); processingRequests_.pop(); - if (!request->controls().contains(controls::draft::TestPatternMode)) + const auto &testPatternMode = request->controls().get(controls::draft::TestPatternMode); + if (!testPatternMode) return; - const int32_t testPatternMode = *(request->controls().get(controls::draft::TestPatternMode)); - int ret = cio2_.sensor()->setTestPatternMode( - static_cast<controls::draft::TestPatternModeEnum>(testPatternMode)); + static_cast<controls::draft::TestPatternModeEnum>(*testPatternMode)); if (ret) { LOG(IPU3, Error) << "Failed to set test pattern mode: " << ret; @@ -1469,7 +1468,7 @@ void IPU3CameraData::frameStart(uint32_t sequence) } request->metadata().set(controls::draft::TestPatternMode, - testPatternMode); + *testPatternMode); } REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3) diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 28f054cb84c8..2e19531eecc4 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1716,16 +1716,15 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList & * Inform the sensor of the latest colour gains if it has the * V4L2_CID_NOTIFY_GAINS control (which means notifyGainsUnity_ is set). */ - if (notifyGainsUnity_ && controls.contains(libcamera::controls::ColourGains)) { - libcamera::Span<const float> colourGains = - *controls.get(libcamera::controls::ColourGains); + const auto &colourGains = controls.get(libcamera::controls::ColourGains); + if (notifyGainsUnity_ && colourGains) { /* The control wants linear gains in the order B, Gb, Gr, R. */ ControlList ctrls(sensor_->controls()); std::array<int32_t, 4> gains{ - static_cast<int32_t>(colourGains[1] * *notifyGainsUnity_), + static_cast<int32_t>((*colourGains)[1] * *notifyGainsUnity_), *notifyGainsUnity_, *notifyGainsUnity_, - static_cast<int32_t>(colourGains[0] * *notifyGainsUnity_) + static_cast<int32_t>((*colourGains)[0] * *notifyGainsUnity_) }; ctrls.set(V4L2_CID_NOTIFY_GAINS, Span<const int32_t>{ gains }); @@ -2052,8 +2051,9 @@ Rectangle RPiCameraData::scaleIspCrop(const Rectangle &ispCrop) const void RPiCameraData::applyScalerCrop(const ControlList &controls) { - if (controls.contains(controls::ScalerCrop)) { - Rectangle nativeCrop = *controls.get<Rectangle>(controls::ScalerCrop); + const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop); + if (scalerCrop) { + Rectangle nativeCrop = *scalerCrop; if (!nativeCrop.width || !nativeCrop.height) nativeCrop = { 0, 0, 1, 1 }; diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp index 4b5d8276564f..5896485cda5b 100644 --- a/src/qcam/dng_writer.cpp +++ b/src/qcam/dng_writer.cpp @@ -391,9 +391,9 @@ int DNGWriter::write(const char *filename, const Camera *camera, TIFFSetField(tif, TIFFTAG_FILLORDER, FILLORDER_MSB2LSB); TIFFSetField(tif, TIFFTAG_MAKE, "libcamera"); - if (cameraProperties.contains(properties::Model)) { - std::string model = *cameraProperties.get(properties::Model); - TIFFSetField(tif, TIFFTAG_MODEL, model.c_str()); + const auto &model = cameraProperties.get(properties::Model); + if (model) { + TIFFSetField(tif, TIFFTAG_MODEL, model->c_str()); /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */ } @@ -437,16 +437,18 @@ int DNGWriter::write(const char *filename, const Camera *camera, */ const double eps = 1e-2; - if (metadata.contains(controls::ColourGains)) { - const auto &colourGains = metadata.get(controls::ColourGains); + const auto &colourGains = metadata.get(controls::ColourGains); + if (colourGains) { if ((*colourGains)[0] > eps && (*colourGains)[1] > eps) { wbGain = Matrix3d::diag((*colourGains)[0], 1, (*colourGains)[1]); neutral[0] = 1.0 / (*colourGains)[0]; /* red */ neutral[2] = 1.0 / (*colourGains)[1]; /* blue */ } } - if (metadata.contains(controls::ColourCorrectionMatrix)) { - Matrix3d ccmSupplied(*metadata.get(controls::ColourCorrectionMatrix)); + + const auto &ccmControl = metadata.get(controls::ColourCorrectionMatrix); + if (ccmControl) { + Matrix3d ccmSupplied(*ccmControl); if (ccmSupplied.determinant() > eps) ccm = ccmSupplied; } @@ -513,9 +515,9 @@ int DNGWriter::write(const char *filename, const Camera *camera, float blackLevel[] = { 0.0f, 0.0f, 0.0f, 0.0f }; uint32_t whiteLevel = (1 << info->bitsPerSample) - 1; - if (metadata.contains(controls::SensorBlackLevels)) { - Span<const int32_t> levels = - *metadata.get(controls::SensorBlackLevels); + const auto &blackLevels = metadata.get(controls::SensorBlackLevels); + if (blackLevels) { + Span<const int32_t> levels = *blackLevels; /* * The black levels control is specified in R, Gr, Gb, B order. @@ -592,16 +594,15 @@ int DNGWriter::write(const char *filename, const Camera *camera, TIFFSetField(tif, EXIFTAG_DATETIMEORIGINAL, strTime); TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime); - if (metadata.contains(controls::AnalogueGain)) { - float gain = *metadata.get(controls::AnalogueGain); - uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f); + const auto &analogGain = metadata.get(controls::AnalogueGain); + if (analogGain) { + uint16_t iso = std::min(std::max(*analogGain * 100, 0.0f), 65535.0f); TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso); } - if (metadata.contains(controls::ExposureTime)) { - float exposureTime = *metadata.get(controls::ExposureTime) / 1e6; - TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime); - } + const auto &exposureTime = metadata.get(controls::ExposureTime); + if (exposureTime) + TIFFSetField(tif, EXIFTAG_EXPOSURETIME, *exposureTime / 1e6); TIFFWriteCustomDirectory(tif, &exifIFDOffset);
Now that the ControlList::get() function returns an instance of std::optional<>, we can replace the ControlList::contains() calls with a nullopt check on the return value of get(). This avoids double lookups of controls through the code base. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- This patch applies on top of "[PATCH v10 1/2] libcamera: controls: Use std::optional to handle invalid control values". --- src/android/camera_capabilities.cpp | 11 ++--- src/android/camera_device.cpp | 46 +++++++++---------- src/android/camera_hal_manager.cpp | 6 +-- src/cam/main.cpp | 5 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 25 +++++----- .../pipeline/raspberrypi/raspberrypi.cpp | 14 +++--- src/qcam/dng_writer.cpp | 35 +++++++------- 7 files changed, 68 insertions(+), 74 deletions(-)