[libcamera-devel] [PATCH] libcamera: controls: Avoid double lookups
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jul 14 10:55:23 CEST 2022
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 at ideasonboard.com>
>
> Reviewed-by: Umang Jain <umang.jain at 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);
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list