[libcamera-devel] [PATCH v6 1/5] libcamera: controls: Use std::optional to handle invalid control values
Jacopo Mondi
jacopo at jmondi.org
Sun Jun 5 11:13:32 CEST 2022
Hi Christian
On Sat, Jun 04, 2022 at 09:50:27PM +0100, Christian Rauch via libcamera-devel wrote:
> Hi Jacopo,
>
> Am 04.06.22 um 09:29 schrieb Jacopo Mondi:
> > Hi Christian,
> > spoke too soon
> >
> > On Sat, Jun 04, 2022 at 09:31:13AM +0200, Jacopo Mondi via libcamera-devel wrote:
> >> Hi Christian
> >>
> >> On Fri, Jun 03, 2022 at 11:07:08PM +0100, Christian Rauch via libcamera-devel wrote:
> >>> Previously, ControlList::get<T>() would use default constructed objects to
> >>> indicate that a ControlList does not have the requested Control. This has
> >>> several disadvantages: 1) It requires types to be default constructible,
> >>> 2) it does not differentiate between a default constructed object and an
> >>> object that happens to have the same state as a default constructed object.
> >>>
> >>> std::optional<T> additionally stores the information if the object is valid
> >>> or not, and therefore is more expressive than a default constructed object.
> >>>
> >>> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
> >>> ---
> >>> include/libcamera/controls.h | 5 +++--
> >>> src/android/camera_capabilities.cpp | 8 +++----
> >>> src/android/camera_device.cpp | 21 +++++++++----------
> >>> src/android/camera_hal_manager.cpp | 2 +-
> >>> src/cam/main.cpp | 4 ++--
> >>> src/ipa/raspberrypi/raspberrypi.cpp | 2 +-
> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++++----
> >>> .../pipeline/raspberrypi/raspberrypi.cpp | 9 ++++----
> >>> src/qcam/dng_writer.cpp | 15 +++++++------
> >>> 9 files changed, 39 insertions(+), 36 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >>> index 665bcac1..192be784 100644
> >>> --- a/include/libcamera/controls.h
> >>> +++ b/include/libcamera/controls.h
> >>> @@ -8,6 +8,7 @@
> >>> #pragma once
> >>>
> >>> #include <assert.h>
> >>> +#include <optional>
> >>> #include <set>
> >>> #include <stdint.h>
> >>> #include <string>
> >>> @@ -373,11 +374,11 @@ public:
> >>> bool contains(unsigned int id) const;
> >>>
> >>> template<typename T>
> >>> - T get(const Control<T> &ctrl) const
> >>> + std::optional<T> get(const Control<T> &ctrl) const
> >>> {
> >>> const ControlValue *val = find(ctrl.id());
> >>> if (!val)
> >>> - return T{};
> >>> + return std::nullopt;
> >>>
> >>> return val->get<T>();
> >>> }
> >>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> >>> index 6f197eb8..892bbc2b 100644
> >>> --- a/src/android/camera_capabilities.cpp
> >>> +++ b/src/android/camera_capabilities.cpp
> >>> @@ -1042,7 +1042,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>> /* Sensor static metadata. */
> >>> std::array<int32_t, 2> pixelArraySize;
> >>> {
> >>> - const Size &size = properties.get(properties::PixelArraySize);
> >>> + const Size &size = properties.get(properties::PixelArraySize).value_or(Size{});
> >>
> >> We should probably wrap this and a few similar cases above with an
> >>
> >> if (properties.contains(...))
> >>
> >> Not for this patch though.
> >>
> >>> pixelArraySize[0] = size.width;
> >>> pixelArraySize[1] = size.height;
> >>> staticMetadata_->addEntry(ANDROID_SENSOR_INFO_PIXEL_ARRAY_SIZE,
> >>> @@ -1050,7 +1050,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>> }
> >>>
> >>> if (properties.contains(properties::UnitCellSize)) {
> >>> - const Size &cellSize = properties.get<Size>(properties::UnitCellSize);
> >>> + const Size &cellSize = *properties.get<Size>(properties::UnitCellSize);
> >
> > Here you could remove the <Size> template argument if I'm not
> > mistaken. I know it was already there...
> >
> > But, most important, I get the following with clang 14.0.0
> >
> > (Showing with both <Size> and without)
> >
> > src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]
> > const Size &cellSize = *properties.get(properties::UnitCellSize);
> >
> > src/android/camera_capabilities.cpp:1053:27: error: object backing the pointer will be destroyed at the end of the full-expression [-Werror,-Wdangling-gsl]
> > const Size &cellSize = *properties.get<Size>(properties::UnitCellSize);
> >
> > I guess the issue might be due to taking a reference to a temporary
> > value as:
> > const Size cellSize = *properties.get(properties::UnitCellSize);
> >
> > as well as:
> > const Size &cellSize = properties.get(properties::UnitCellSize).value_or(Size{});;
> >
> > compile fine.
>
> Thanks for pointing this out. I could reproduce this with clang
> ("CC=clang CXX=clang++ meson build -Dandroid=enabled") but not with gcc.
> Even "-Dwarning_level=3" did not show these errors.
>
> Does libcamera run a build server somewhere? This would be helpful to
> discover errors in non-standard configurations (e.g. with clang in this
> case or with "android" enabled).
>
Unfortunately not for public use. I run a buildbot instance as a best
effort service for internal (aka people who can push to master)
pre-integration testing. It's not stable or maintained well enough
for public usage yet.
We do also have the linux-media Jenkins instance, but that only runs
on master afaict
> >
> > When it comes to the usage of value_or(), its documentation says:
> > [1] https://en.cppreference.com/w/cpp/utility/optional/value_or
> > -T must meet the requirements of CopyConstructible in order to use overload (1).
> > -T must meet the requirements of MoveConstructible in order to use overload (2).
> >
> > which means the value is copied or moved back while operator*
> > [2] https://en.cppreference.com/w/cpp/utility/optional/operator*
> > 1) Returns a pointer to the contained value.
> > 2) Returns a reference to the contained value.
> >
> > I guess the issue is due to trying to reference the content of a
> > temporary std::optional<T> instance, created to wrap the return value
> > of ControlValue::get<T>
> >
> > ControlList:
> > template<typename T>
> > std::optional<T> get(const Control<T> &ctrl) const
> > {
> > const ControlValue *val = find(ctrl.id());
> > if (!val)
> > return std::nullopt;
> >
> > return val->get<T>();
> > }
> >
> > This should be fine, as the object returned by ControlValue::get<>()
> > refers to valid memory, and as we could take references to it before
> > this series, we should be able to do so now, but the compiler worries
> > that dereferencing the temporary std::optional<> would leave us with a
> > pointer to invalid memory. Is my interpretation of the issue correct ?
> >
>
> Yes, I think what is happening here is that the optional returns the
> pointer to a value that can become "invalid" later. This could happen
> when the "std::optional<T>::reset()" is called or when "std::nullopt" is
> assigned.
>
> Taking the reference of this would be dangerous. We know that the
> reference will "cellSize" be destroyed shortly after, but generally,
> there is no guarantee. For the compiler, it makes sense to complain
> about this.
>
> I only see two ways to solve this:
>
> 1. copy the content:
>
> const Size cellSize = *properties.get<Size>(properties::UnitCellSize);
>
> 2. use the "std::optional" directly:
>
> const auto cellSize = properties.get<Size>(properties::UnitCellSize);
>
> and then access it via the "->" operator:
>
> cellSize->width
>
> I personally prefer the second option since it avoids copying the entire
> content, which could be expensive for other control types.
>
I agree copies should be avoided as they can become expensive for
larger objects. We also have the issue that using value_or() might
introduce a copy operation that might get unnoticed..
Using std::optional<> directly might get a bit heavy for controls of
type Span<>
const auto &rects = properties.get(properties::PixelArrayActiveAreas);
std::vector<int32_t> data{
static_cast<int32_t>((*rects)[0].x),
static_cast<int32_t>((*rects)[0].y),
static_cast<int32_t>((*rects)[0].width),
static_cast<int32_t>((*rects)[0].height),
};
Specifically because of the need to dereference the std::optiona<>
before accessing its content with operator[]
It's not something huge, and I still think the series has ways more
pro than cons, but I wonder if we shouln't wrap std::optional<> in
some libcamera::controls specific type and expose that in the public
API instead of the raw std::optional<>
Not asking you to do so for this series of course.
Thanks
j
>
> >>> cellSize.width * pixelArraySize[0] / 1e6f,
> >>> cellSize.height * pixelArraySize[1] / 1e6f
> >>> @@ -1061,7 +1061,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>>
> >>> {
> >>> const Span<const Rectangle> &rects =
> >>> - properties.get(properties::PixelArrayActiveAreas);
> >>> + properties.get(properties::PixelArrayActiveAreas).value_or(Span<const Rectangle>{});
> >>> std::vector<int32_t> data{
> >>> static_cast<int32_t>(rects[0].x),
> >>> static_cast<int32_t>(rects[0].y),
> >>> @@ -1080,7 +1080,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>>
> >>> /* Report the color filter arrangement if the camera reports it. */
> >>> if (properties.contains(properties::draft::ColorFilterArrangement)) {
> >>> - uint8_t filterArr = properties.get(properties::draft::ColorFilterArrangement);
> >>> + uint8_t filterArr = *properties.get(properties::draft::ColorFilterArrangement);
> >>> staticMetadata_->addEntry(ANDROID_SENSOR_INFO_COLOR_FILTER_ARRANGEMENT,
> >>> filterArr);
> >>> }
> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>> index 8e804d4d..ec117101 100644
> >>> --- a/src/android/camera_device.cpp
> >>> +++ b/src/android/camera_device.cpp
> >>> @@ -305,7 +305,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> >>> const ControlList &properties = camera_->properties();
> >>>
> >>> if (properties.contains(properties::Location)) {
> >>> - int32_t location = properties.get(properties::Location);
> >>> + int32_t location = *properties.get(properties::Location);
> >>> switch (location) {
> >>> case properties::CameraLocationFront:
> >>> facing_ = CAMERA_FACING_FRONT;
> >>> @@ -355,7 +355,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> >>> * metadata.
> >>> */
> >>> if (properties.contains(properties::Rotation)) {
> >>> - int rotation = properties.get(properties::Rotation);
> >>> + int rotation = *properties.get(properties::Rotation);
> >>> orientation_ = (360 - rotation) % 360;
> >>> if (cameraConfigData && cameraConfigData->rotation != -1 &&
> >>> orientation_ != cameraConfigData->rotation) {
> >>> @@ -1094,7 +1094,8 @@ void CameraDevice::requestComplete(Request *request)
> >>> * as soon as possible, earlier than request completion time.
> >>> */
> >>> uint64_t sensorTimestamp = static_cast<uint64_t>(request->metadata()
> >>> - .get(controls::SensorTimestamp));
> >>> + .get(controls::SensorTimestamp)
> >>> + .value_or(0));
> >>> notifyShutter(descriptor->frameNumber_, sensorTimestamp);
> >>>
> >>> LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> >>> @@ -1473,29 +1474,28 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> >>> rolling_shutter_skew);
> >>>
> >>> /* Add metadata tags reported by libcamera. */
> >>> - const int64_t timestamp = metadata.get(controls::SensorTimestamp);
> >>> + 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);
> >>> + uint8_t pipeline_depth = *metadata.get<int32_t>(controls::draft::PipelineDepth);
> >>> resultMetadata->addEntry(ANDROID_REQUEST_PIPELINE_DEPTH,
> >>> pipeline_depth);
> >>> }
> >>>
> >>> if (metadata.contains(controls::ExposureTime)) {
> >>> - int64_t exposure = metadata.get(controls::ExposureTime) * 1000ULL;
> >>> + int64_t exposure = *metadata.get(controls::ExposureTime) * 1000ULL;
> >>> resultMetadata->addEntry(ANDROID_SENSOR_EXPOSURE_TIME, exposure);
> >>> }
> >>>
> >>> if (metadata.contains(controls::FrameDuration)) {
> >>> - int64_t duration = metadata.get(controls::FrameDuration) * 1000;
> >>> + int64_t duration = *metadata.get(controls::FrameDuration) * 1000;
> >>> resultMetadata->addEntry(ANDROID_SENSOR_FRAME_DURATION,
> >>> duration);
> >>> }
> >>>
> >>> if (metadata.contains(controls::ScalerCrop)) {
> >>> - Rectangle crop = metadata.get(controls::ScalerCrop);
> >>> + Rectangle crop = *metadata.get(controls::ScalerCrop);
> >>> int32_t cropRect[] = {
> >>> crop.x, crop.y, static_cast<int32_t>(crop.width),
> >>> static_cast<int32_t>(crop.height),
> >>> @@ -1504,8 +1504,7 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons
> >>> }
> >>>
> >>> if (metadata.contains(controls::draft::TestPatternMode)) {
> >>> - const int32_t testPatternMode =
> >>> - metadata.get(controls::draft::TestPatternMode);
> >>> + const int32_t testPatternMode = *metadata.get(controls::draft::TestPatternMode);
> >>> resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,
> >>> testPatternMode);
> >>> }
> >>> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> >>> index 5f7bfe26..0b23397a 100644
> >>> --- a/src/android/camera_hal_manager.cpp
> >>> +++ b/src/android/camera_hal_manager.cpp
> >>> @@ -232,7 +232,7 @@ int32_t CameraHalManager::cameraLocation(const Camera *cam)
> >>> if (!properties.contains(properties::Location))
> >>> return -1;
> >>>
> >>> - return properties.get(properties::Location);
> >>> + return properties.get(properties::Location).value_or(0);
> >>
> >> No need to value_or() as we return earlier if Location is not
> >> available
> >>
> >>> }
> >>>
> >>> CameraDevice *CameraHalManager::cameraDeviceFromHalId(unsigned int id)
> >>> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> >>> index 79875ed7..d8115cd8 100644
> >>> --- a/src/cam/main.cpp
> >>> +++ b/src/cam/main.cpp
> >>> @@ -301,7 +301,7 @@ std::string CamApp::cameraName(const Camera *camera)
> >>> * is only used if the location isn't present or is set to External.
> >>> */
> >>> if (props.contains(properties::Location)) {
> >>> - switch (props.get(properties::Location)) {
> >>> + switch (*props.get(properties::Location)) {
> >>> case properties::CameraLocationFront:
> >>> addModel = false;
> >>> name = "Internal front camera ";
> >>> @@ -321,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)
> >>> * If the camera location is not availble use the camera model
> >>> * to build the camera name.
> >>> */
> >>> - name = "'" + props.get(properties::Model) + "' ";
> >>> + name = "'" + *props.get(properties::Model) + "' ";
> >>> }
> >>>
> >>> name += "(" + camera->id() + ")";
> >>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> >>> index 3b126bb5..f65a0680 100644
> >>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >>> @@ -939,7 +939,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> >>>
> >>> void IPARPi::prepareISP(const ISPConfig &data)
> >>> {
> >>> - int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp);
> >>> + int64_t frameTimestamp = data.controls.get(controls::SensorTimestamp).value_or(0);
> >>
> >> This should in theory be guaranteed to be there, as it's the pipeline
> >> handler that populates it. However I'm not sure what's best. Either
> >> ASSERT() that the control is there or default it to 0. frameTimestamp
> >> is not used as a divider anywhere so there's no risk of undefined
> >> behaviour.
> >>
> >>> RPiController::Metadata lastMetadata;
> >>> Span<uint8_t> embeddedBuffer;
> >>>
> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> index fd989e61..53332826 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -1145,7 +1145,7 @@ int PipelineHandlerIPU3::registerCameras()
> >>> /* Convert the sensor rotation to a transformation */
> >>> int32_t rotation = 0;
> >>> if (data->properties_.contains(properties::Rotation))
> >>> - rotation = data->properties_.get(properties::Rotation);
> >>> + rotation = *(data->properties_.get(properties::Rotation));
> >>> else
> >>> LOG(IPU3, Warning) << "Rotation control not exposed by "
> >>> << cio2->sensor()->id()
> >>> @@ -1331,7 +1331,7 @@ 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);
> >>> + cropRegion_ = *(request->controls().get(controls::ScalerCrop));
> >>> request->metadata().set(controls::ScalerCrop, cropRegion_);
> >>>
> >>> if (frameInfos_.tryComplete(info))
> >>> @@ -1424,7 +1424,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
> >>> return;
> >>> }
> >>>
> >>> - ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp),
> >>> + ipa_->processStatsBuffer(info->id, request->metadata().get(controls::SensorTimestamp).value_or(0),
> >>> info->statBuffer->cookie(), info->effectiveSensorControls);
> >>
> >> Also in this case we should be guaranteed the sensor timestamp is
> >> there.
> >>
> >> I'm tempted to ASSERT here as well...
> >>
> >>> }
> >>>
> >>> @@ -1458,8 +1458,7 @@ void IPU3CameraData::frameStart(uint32_t sequence)
> >>> if (!request->controls().contains(controls::draft::TestPatternMode))
> >>> return;
> >>>
> >>> - const int32_t testPatternMode = request->controls().get(
> >>> - controls::draft::TestPatternMode);
> >>> + const int32_t testPatternMode = request->controls().get(controls::draft::TestPatternMode).value_or(0);
> >>
> >> No need as we return earlier if !TestPatternMode
> >>
> >>>
> >>> int ret = cio2_.sensor()->setTestPatternMode(
> >>> static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> index adc397e8..a62afdd4 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> @@ -365,7 +365,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> >>> * error means the platform can never run. Let's just print a warning
> >>> * and continue regardless; the rotation is effectively set to zero.
> >>> */
> >>> - int32_t rotation = data_->sensor_->properties().get(properties::Rotation);
> >>> + int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(0);
> >>> bool success;
> >>> Transform rotationTransform = transformFromRotation(rotation, &success);
> >>> if (!success)
> >>> @@ -1706,7 +1706,8 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
> >>> * 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);
> >>> + libcamera::Span<const float> colourGains =
> >>> + *controls.get(libcamera::controls::ColourGains);
> >>> /* The control wants linear gains in the order B, Gb, Gr, R. */
> >>> ControlList ctrls(sensor_->controls());
> >>> std::array<int32_t, 4> gains{
> >>> @@ -2041,7 +2042,7 @@ 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);
> >>> + Rectangle nativeCrop = *controls.get<Rectangle>(controls::ScalerCrop);
> >>>
> >>> if (!nativeCrop.width || !nativeCrop.height)
> >>> nativeCrop = { 0, 0, 1, 1 };
> >>> @@ -2079,7 +2080,7 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
> >>> Request *request)
> >>> {
> >>> request->metadata().set(controls::SensorTimestamp,
> >>> - bufferControls.get(controls::SensorTimestamp));
> >>> + bufferControls.get(controls::SensorTimestamp).value_or(0));
> >>>
> >>> request->metadata().set(controls::ScalerCrop, scalerCrop_);
> >>> }
> >>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> >>> index 34c8df5a..780f58f7 100644
> >>> --- a/src/qcam/dng_writer.cpp
> >>> +++ b/src/qcam/dng_writer.cpp
> >>> @@ -392,7 +392,7 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >>> TIFFSetField(tif, TIFFTAG_MAKE, "libcamera");
> >>>
> >>> if (cameraProperties.contains(properties::Model)) {
> >>> - std::string model = cameraProperties.get(properties::Model);
> >>> + std::string model = *cameraProperties.get(properties::Model);
> >>> TIFFSetField(tif, TIFFTAG_MODEL, model.c_str());
> >>> /* \todo set TIFFTAG_UNIQUECAMERAMODEL. */
> >>> }
> >>> @@ -438,7 +438,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >>> const double eps = 1e-2;
> >>>
> >>> if (metadata.contains(controls::ColourGains)) {
> >>> - Span<const float> const &colourGains = metadata.get(controls::ColourGains);
> >>> + Span<const float> const &colourGains =
> >>> + *metadata.get(controls::ColourGains);
> >>> if (colourGains[0] > eps && colourGains[1] > eps) {
> >>> wbGain = Matrix3d::diag(colourGains[0], 1, colourGains[1]);
> >>> neutral[0] = 1.0 / colourGains[0]; /* red */
> >>> @@ -446,7 +447,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >>> }
> >>> }
> >>> if (metadata.contains(controls::ColourCorrectionMatrix)) {
> >>> - Span<const float> const &coeffs = metadata.get(controls::ColourCorrectionMatrix);
> >>> + Span<const float> const &coeffs =
> >>> + *metadata.get(controls::ColourCorrectionMatrix);
> >>> Matrix3d ccmSupplied(coeffs);
> >>> if (ccmSupplied.determinant() > eps)
> >>> ccm = ccmSupplied;
> >>> @@ -515,7 +517,8 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >>> uint32_t whiteLevel = (1 << info->bitsPerSample) - 1;
> >>>
> >>> if (metadata.contains(controls::SensorBlackLevels)) {
> >>> - Span<const int32_t> levels = metadata.get(controls::SensorBlackLevels);
> >>> + Span<const int32_t> levels =
> >>> + *metadata.get(controls::SensorBlackLevels);
> >>>
> >>> /*
> >>> * The black levels control is specified in R, Gr, Gb, B order.
> >>> @@ -593,13 +596,13 @@ int DNGWriter::write(const char *filename, const Camera *camera,
> >>> TIFFSetField(tif, EXIFTAG_DATETIMEDIGITIZED, strTime);
> >>>
> >>> if (metadata.contains(controls::AnalogueGain)) {
> >>> - float gain = metadata.get(controls::AnalogueGain);
> >>> + float gain = *metadata.get(controls::AnalogueGain);
> >>> uint16_t iso = std::min(std::max(gain * 100, 0.0f), 65535.0f);
> >>> TIFFSetField(tif, EXIFTAG_ISOSPEEDRATINGS, 1, &iso);
> >>> }
> >>>
> >>> if (metadata.contains(controls::ExposureTime)) {
> >>> - float exposureTime = metadata.get(controls::ExposureTime) / 1e6;
> >>> + float exposureTime = *metadata.get(controls::ExposureTime) / 1e6;
> >>> TIFFSetField(tif, EXIFTAG_EXPOSURETIME, exposureTime);
> >>> }
> >>
> >> Let's see what's Laurent comment on the possible assertion, but the
> >> patch looks good to me and the few other nits can be changed when
> >> applying if you agree with them
> >>
> >> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >>
> >> Thanks
> >> j
> >>
> >>>
> >>> --
> >>> 2.34.1
> >>>
More information about the libcamera-devel
mailing list