Message ID | 20220802210324.115893-1-Rauch.Christian@gmx.de |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Christian, On Tue, Aug 02, 2022 at 11:03:24PM +0200, Christian Rauch wrote: > Define Span types explicitly as either variable- or fixed-sized. This > introduces a new convention for defining Span dimensions in the property > and control value definitions and generates Span types as variable-sized > Span<T> or as fixed-sized Span<T,N>. > > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> The issue with gcc8 seems now solved. One more tag and I hope we can collect the patch ? Thanks j > --- > include/libcamera/controls.h | 2 +- > src/ipa/raspberrypi/raspberrypi.cpp | 19 +++++++++-------- > src/libcamera/control_ids.yaml | 4 ++-- > src/libcamera/property_ids.yaml | 4 ++-- > src/qcam/dng_writer.cpp | 2 +- > utils/gen-controls.py | 32 ++++++++++++++++++++--------- > 6 files changed, 38 insertions(+), 25 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index b1b52acb..ebc168fc 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -168,7 +168,7 @@ public: > > using V = typename T::value_type; > const V *value = reinterpret_cast<const V *>(data().data()); > - return { value, numElements_ }; > + return T{ value, numElements_ }; > } > > #ifndef __DOXYGEN__ > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 6befdd71..69c73f8c 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -567,18 +567,19 @@ void IPARPi::reportMetadata() > > AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status"); > if (awbStatus) { > - libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gainR), > - static_cast<float>(awbStatus->gainB) }); > + libcameraMetadata_.set(controls::ColourGains, > + Span<const float, 2>({ static_cast<float>(awbStatus->gainR), > + static_cast<float>(awbStatus->gainB) })); > libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK); > } > > BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status"); > if (blackLevelStatus) > libcameraMetadata_.set(controls::SensorBlackLevels, > - { static_cast<int32_t>(blackLevelStatus->blackLevelR), > - static_cast<int32_t>(blackLevelStatus->blackLevelG), > - static_cast<int32_t>(blackLevelStatus->blackLevelG), > - static_cast<int32_t>(blackLevelStatus->blackLevelB) }); > + Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR), > + static_cast<int32_t>(blackLevelStatus->blackLevelG), > + static_cast<int32_t>(blackLevelStatus->blackLevelG), > + static_cast<int32_t>(blackLevelStatus->blackLevelB) })); > > FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status"); > if (focusStatus && focusStatus->num == 12) { > @@ -883,7 +884,7 @@ void IPARPi::queueRequest(const ControlList &controls) > if (gains[0] != 0.0f && gains[1] != 0.0f) > /* A gain of 0.0f will switch back to auto mode. */ > libcameraMetadata_.set(controls::ColourGains, > - { gains[0], gains[1] }); > + Span<const float, 2>({ gains[0], gains[1] })); > break; > } > > @@ -1167,8 +1168,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur > > /* Return the validated limits via metadata. */ > libcameraMetadata_.set(controls::FrameDurationLimits, > - { static_cast<int64_t>(minFrameDuration_.get<std::micro>()), > - static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }); > + Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()), > + static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) })); > > /* > * Calculate the maximum exposure time possible for the AGC to use. > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > index ecab3ae9..5fa168c6 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -291,7 +291,7 @@ controls: > transformation. The 3x3 matrix is stored in conventional reading > order in an array of 9 floating point values. > > - size: [3x3] > + size: [3,3] > > - ScalerCrop: > type: Rectangle > @@ -525,7 +525,7 @@ controls: > the window where the focal distance for the objects shown in that part > of the image are closest to the camera. > > - size: [n] > + size: [] > > - AfTrigger: > type: int32_t > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > index 11b7ebdc..a87485d7 100644 > --- a/src/libcamera/property_ids.yaml > +++ b/src/libcamera/property_ids.yaml > @@ -497,7 +497,7 @@ controls: > > - PixelArrayOpticalBlackRectangles: > type: Rectangle > - size: [n] > + size: [] > description: | > The pixel array region(s) which contain optical black pixels > considered valid for calibration purposes. > @@ -592,7 +592,7 @@ controls: > > - PixelArrayActiveAreas: > type: Rectangle > - size: [n] > + size: [] > description: | > The PixelArrayActiveAreas property defines the (possibly multiple and > overlapping) portions of the camera sensor readable pixel matrix > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp > index 5b94b3f2..b4362537 100644 > --- a/src/qcam/dng_writer.cpp > +++ b/src/qcam/dng_writer.cpp > @@ -517,7 +517,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > const auto &blackLevels = metadata.get(controls::SensorBlackLevels); > if (blackLevels) { > - Span<const int32_t> levels = *blackLevels; > + Span<const int32_t, 4> levels = *blackLevels; > > /* > * The black levels control is specified in R, Gr, Gb, B order. > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > index 3f99b5e2..46ba4394 100755 > --- a/utils/gen-controls.py > +++ b/utils/gen-controls.py > @@ -7,6 +7,8 @@ > # gen-controls.py - Generate control definitions from YAML > > import argparse > +from functools import reduce > +import operator > import string > import sys > import yaml > @@ -22,6 +24,24 @@ def format_description(description): > return '\n'.join([(line and ' * ' or ' *') + line for line in description]) > > > +def get_ctrl_type(ctrl): > + ctrl_type = ctrl['type'] > + ctrl_size_arr = ctrl.get('size') > + > + if ctrl_type == 'string': > + return 'std::string' > + elif ctrl_size_arr is not None: > + if len(ctrl_size_arr) > 0: > + # fixed-sized Span > + ctrl_span_size = reduce(operator.mul, ctrl_size_arr) > + return f"Span<const {ctrl_type}, {ctrl_span_size}>" > + else: > + # variable-sized Span > + return f"Span<const {ctrl_type}>" > + else: > + return ctrl_type > + > + > def generate_cpp(controls): > enum_doc_start_template = string.Template('''/** > * \\enum ${name}Enum > @@ -50,11 +70,7 @@ ${description} > name, ctrl = ctrl.popitem() > id_name = snake_case(name).upper() > > - ctrl_type = ctrl['type'] > - if ctrl_type == 'string': > - ctrl_type = 'std::string' > - elif ctrl.get('size'): > - ctrl_type = 'Span<const %s>' % ctrl_type > + ctrl_type = get_ctrl_type(ctrl) > > info = { > 'name': name, > @@ -135,11 +151,7 @@ def generate_h(controls): > > ids.append('\t' + id_name + ' = ' + str(id_value) + ',') > > - ctrl_type = ctrl['type'] > - if ctrl_type == 'string': > - ctrl_type = 'std::string' > - elif ctrl.get('size'): > - ctrl_type = 'Span<const %s>' % ctrl_type > + ctrl_type = get_ctrl_type(ctrl) > > info = { > 'name': name, > -- > 2.34.1 >
Quoting Jacopo Mondi via libcamera-devel (2022-08-03 10:44:01) > Hi Christian, > > On Tue, Aug 02, 2022 at 11:03:24PM +0200, Christian Rauch wrote: > > Define Span types explicitly as either variable- or fixed-sized. This > > introduces a new convention for defining Span dimensions in the property > > and control value definitions and generates Span types as variable-sized > > Span<T> or as fixed-sized Span<T,N>. > > > > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > The issue with gcc8 seems now solved. > > One more tag and I hope we can collect the patch ? What are the user facing API changes here? (i.e. how much does this break applications?) I'm surprised there are no updates to the python bindings in this patch ? Is that all handled gracefully? (Is it tested?) -- Kieran > > Thanks > j > > > --- > > include/libcamera/controls.h | 2 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 19 +++++++++-------- > > src/libcamera/control_ids.yaml | 4 ++-- > > src/libcamera/property_ids.yaml | 4 ++-- > > src/qcam/dng_writer.cpp | 2 +- > > utils/gen-controls.py | 32 ++++++++++++++++++++--------- > > 6 files changed, 38 insertions(+), 25 deletions(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index b1b52acb..ebc168fc 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -168,7 +168,7 @@ public: > > > > using V = typename T::value_type; > > const V *value = reinterpret_cast<const V *>(data().data()); > > - return { value, numElements_ }; > > + return T{ value, numElements_ }; > > } > > > > #ifndef __DOXYGEN__ > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index 6befdd71..69c73f8c 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -567,18 +567,19 @@ void IPARPi::reportMetadata() > > > > AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status"); > > if (awbStatus) { > > - libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gainR), > > - static_cast<float>(awbStatus->gainB) }); > > + libcameraMetadata_.set(controls::ColourGains, > > + Span<const float, 2>({ static_cast<float>(awbStatus->gainR), > > + static_cast<float>(awbStatus->gainB) })); > > libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK); > > } > > > > BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status"); > > if (blackLevelStatus) > > libcameraMetadata_.set(controls::SensorBlackLevels, > > - { static_cast<int32_t>(blackLevelStatus->blackLevelR), > > - static_cast<int32_t>(blackLevelStatus->blackLevelG), > > - static_cast<int32_t>(blackLevelStatus->blackLevelG), > > - static_cast<int32_t>(blackLevelStatus->blackLevelB) }); > > + Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR), > > + static_cast<int32_t>(blackLevelStatus->blackLevelG), > > + static_cast<int32_t>(blackLevelStatus->blackLevelG), > > + static_cast<int32_t>(blackLevelStatus->blackLevelB) })); > > > > FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status"); > > if (focusStatus && focusStatus->num == 12) { > > @@ -883,7 +884,7 @@ void IPARPi::queueRequest(const ControlList &controls) > > if (gains[0] != 0.0f && gains[1] != 0.0f) > > /* A gain of 0.0f will switch back to auto mode. */ > > libcameraMetadata_.set(controls::ColourGains, > > - { gains[0], gains[1] }); > > + Span<const float, 2>({ gains[0], gains[1] })); > > break; > > } > > > > @@ -1167,8 +1168,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur > > > > /* Return the validated limits via metadata. */ > > libcameraMetadata_.set(controls::FrameDurationLimits, > > - { static_cast<int64_t>(minFrameDuration_.get<std::micro>()), > > - static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }); > > + Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()), > > + static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) })); > > > > /* > > * Calculate the maximum exposure time possible for the AGC to use. > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > index ecab3ae9..5fa168c6 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -291,7 +291,7 @@ controls: > > transformation. The 3x3 matrix is stored in conventional reading > > order in an array of 9 floating point values. > > > > - size: [3x3] > > + size: [3,3] > > > > - ScalerCrop: > > type: Rectangle > > @@ -525,7 +525,7 @@ controls: > > the window where the focal distance for the objects shown in that part > > of the image are closest to the camera. > > > > - size: [n] > > + size: [] > > > > - AfTrigger: > > type: int32_t > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > > index 11b7ebdc..a87485d7 100644 > > --- a/src/libcamera/property_ids.yaml > > +++ b/src/libcamera/property_ids.yaml > > @@ -497,7 +497,7 @@ controls: > > > > - PixelArrayOpticalBlackRectangles: > > type: Rectangle > > - size: [n] > > + size: [] > > description: | > > The pixel array region(s) which contain optical black pixels > > considered valid for calibration purposes. > > @@ -592,7 +592,7 @@ controls: > > > > - PixelArrayActiveAreas: > > type: Rectangle > > - size: [n] > > + size: [] > > description: | > > The PixelArrayActiveAreas property defines the (possibly multiple and > > overlapping) portions of the camera sensor readable pixel matrix > > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp > > index 5b94b3f2..b4362537 100644 > > --- a/src/qcam/dng_writer.cpp > > +++ b/src/qcam/dng_writer.cpp > > @@ -517,7 +517,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > > > const auto &blackLevels = metadata.get(controls::SensorBlackLevels); > > if (blackLevels) { > > - Span<const int32_t> levels = *blackLevels; > > + Span<const int32_t, 4> levels = *blackLevels; > > > > /* > > * The black levels control is specified in R, Gr, Gb, B order. > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > index 3f99b5e2..46ba4394 100755 > > --- a/utils/gen-controls.py > > +++ b/utils/gen-controls.py > > @@ -7,6 +7,8 @@ > > # gen-controls.py - Generate control definitions from YAML > > > > import argparse > > +from functools import reduce > > +import operator > > import string > > import sys > > import yaml > > @@ -22,6 +24,24 @@ def format_description(description): > > return '\n'.join([(line and ' * ' or ' *') + line for line in description]) > > > > > > +def get_ctrl_type(ctrl): > > + ctrl_type = ctrl['type'] > > + ctrl_size_arr = ctrl.get('size') > > + > > + if ctrl_type == 'string': > > + return 'std::string' > > + elif ctrl_size_arr is not None: > > + if len(ctrl_size_arr) > 0: > > + # fixed-sized Span > > + ctrl_span_size = reduce(operator.mul, ctrl_size_arr) > > + return f"Span<const {ctrl_type}, {ctrl_span_size}>" > > + else: > > + # variable-sized Span > > + return f"Span<const {ctrl_type}>" > > + else: > > + return ctrl_type > > + > > + > > def generate_cpp(controls): > > enum_doc_start_template = string.Template('''/** > > * \\enum ${name}Enum > > @@ -50,11 +70,7 @@ ${description} > > name, ctrl = ctrl.popitem() > > id_name = snake_case(name).upper() > > > > - ctrl_type = ctrl['type'] > > - if ctrl_type == 'string': > > - ctrl_type = 'std::string' > > - elif ctrl.get('size'): > > - ctrl_type = 'Span<const %s>' % ctrl_type > > + ctrl_type = get_ctrl_type(ctrl) > > > > info = { > > 'name': name, > > @@ -135,11 +151,7 @@ def generate_h(controls): > > > > ids.append('\t' + id_name + ' = ' + str(id_value) + ',') > > > > - ctrl_type = ctrl['type'] > > - if ctrl_type == 'string': > > - ctrl_type = 'std::string' > > - elif ctrl.get('size'): > > - ctrl_type = 'Span<const %s>' % ctrl_type > > + ctrl_type = get_ctrl_type(ctrl) > > > > info = { > > 'name': name, > > -- > > 2.34.1 > >
Hi Kieran, Am 03.08.22 um 14:17 schrieb Kieran Bingham: > Quoting Jacopo Mondi via libcamera-devel (2022-08-03 10:44:01) >> Hi Christian, >> >> On Tue, Aug 02, 2022 at 11:03:24PM +0200, Christian Rauch wrote: >>> Define Span types explicitly as either variable- or fixed-sized. This >>> introduces a new convention for defining Span dimensions in the property >>> and control value definitions and generates Span types as variable-sized >>> Span<T> or as fixed-sized Span<T,N>. >>> >>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> >> >> The issue with gcc8 seems now solved. >> >> One more tag and I hope we can collect the patch ? > > What are the user facing API changes here? (i.e. how much does this > break applications?) Everything that touches fixed-sized Spans with explicit types (i.e. no auto) need a change that defines the size. variable-sized spans should behave as before. E.g. in the diff you can see that something like "Span<const int32_t> levels = *blackLevels;" needs to be set the Span size as template paremter: "Span<const int32_t, 4> levels = *blackLevels;" But overall, I only had to change this in 5 places in the entire codebase to make this compile. So I don't think the impact will be that big. > > I'm surprised there are no updates to the python bindings in this patch > ? > > Is that all handled gracefully? (Is it tested?) I compiled ("meson build_py -Dpycamera=enabled") and ran the tests ("ninja -C build_py/ test") without failures. Is there another way to test that the python bindings work? A quick glimpse into "py_controls_generated.cpp" tells me that Spans are not used there. > > -- > Kieran > > >> >> Thanks >> j >> >>> --- >>> include/libcamera/controls.h | 2 +- >>> src/ipa/raspberrypi/raspberrypi.cpp | 19 +++++++++-------- >>> src/libcamera/control_ids.yaml | 4 ++-- >>> src/libcamera/property_ids.yaml | 4 ++-- >>> src/qcam/dng_writer.cpp | 2 +- >>> utils/gen-controls.py | 32 ++++++++++++++++++++--------- >>> 6 files changed, 38 insertions(+), 25 deletions(-) >>> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >>> index b1b52acb..ebc168fc 100644 >>> --- a/include/libcamera/controls.h >>> +++ b/include/libcamera/controls.h >>> @@ -168,7 +168,7 @@ public: >>> >>> using V = typename T::value_type; >>> const V *value = reinterpret_cast<const V *>(data().data()); >>> - return { value, numElements_ }; >>> + return T{ value, numElements_ }; >>> } >>> >>> #ifndef __DOXYGEN__ >>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp >>> index 6befdd71..69c73f8c 100644 >>> --- a/src/ipa/raspberrypi/raspberrypi.cpp >>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp >>> @@ -567,18 +567,19 @@ void IPARPi::reportMetadata() >>> >>> AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status"); >>> if (awbStatus) { >>> - libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gainR), >>> - static_cast<float>(awbStatus->gainB) }); >>> + libcameraMetadata_.set(controls::ColourGains, >>> + Span<const float, 2>({ static_cast<float>(awbStatus->gainR), >>> + static_cast<float>(awbStatus->gainB) })); >>> libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK); >>> } >>> >>> BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status"); >>> if (blackLevelStatus) >>> libcameraMetadata_.set(controls::SensorBlackLevels, >>> - { static_cast<int32_t>(blackLevelStatus->blackLevelR), >>> - static_cast<int32_t>(blackLevelStatus->blackLevelG), >>> - static_cast<int32_t>(blackLevelStatus->blackLevelG), >>> - static_cast<int32_t>(blackLevelStatus->blackLevelB) }); >>> + Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR), >>> + static_cast<int32_t>(blackLevelStatus->blackLevelG), >>> + static_cast<int32_t>(blackLevelStatus->blackLevelG), >>> + static_cast<int32_t>(blackLevelStatus->blackLevelB) })); >>> >>> FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status"); >>> if (focusStatus && focusStatus->num == 12) { >>> @@ -883,7 +884,7 @@ void IPARPi::queueRequest(const ControlList &controls) >>> if (gains[0] != 0.0f && gains[1] != 0.0f) >>> /* A gain of 0.0f will switch back to auto mode. */ >>> libcameraMetadata_.set(controls::ColourGains, >>> - { gains[0], gains[1] }); >>> + Span<const float, 2>({ gains[0], gains[1] })); >>> break; >>> } >>> >>> @@ -1167,8 +1168,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur >>> >>> /* Return the validated limits via metadata. */ >>> libcameraMetadata_.set(controls::FrameDurationLimits, >>> - { static_cast<int64_t>(minFrameDuration_.get<std::micro>()), >>> - static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }); >>> + Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()), >>> + static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) })); >>> >>> /* >>> * Calculate the maximum exposure time possible for the AGC to use. >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml >>> index ecab3ae9..5fa168c6 100644 >>> --- a/src/libcamera/control_ids.yaml >>> +++ b/src/libcamera/control_ids.yaml >>> @@ -291,7 +291,7 @@ controls: >>> transformation. The 3x3 matrix is stored in conventional reading >>> order in an array of 9 floating point values. >>> >>> - size: [3x3] >>> + size: [3,3] >>> >>> - ScalerCrop: >>> type: Rectangle >>> @@ -525,7 +525,7 @@ controls: >>> the window where the focal distance for the objects shown in that part >>> of the image are closest to the camera. >>> >>> - size: [n] >>> + size: [] >>> >>> - AfTrigger: >>> type: int32_t >>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml >>> index 11b7ebdc..a87485d7 100644 >>> --- a/src/libcamera/property_ids.yaml >>> +++ b/src/libcamera/property_ids.yaml >>> @@ -497,7 +497,7 @@ controls: >>> >>> - PixelArrayOpticalBlackRectangles: >>> type: Rectangle >>> - size: [n] >>> + size: [] >>> description: | >>> The pixel array region(s) which contain optical black pixels >>> considered valid for calibration purposes. >>> @@ -592,7 +592,7 @@ controls: >>> >>> - PixelArrayActiveAreas: >>> type: Rectangle >>> - size: [n] >>> + size: [] >>> description: | >>> The PixelArrayActiveAreas property defines the (possibly multiple and >>> overlapping) portions of the camera sensor readable pixel matrix >>> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp >>> index 5b94b3f2..b4362537 100644 >>> --- a/src/qcam/dng_writer.cpp >>> +++ b/src/qcam/dng_writer.cpp >>> @@ -517,7 +517,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, >>> >>> const auto &blackLevels = metadata.get(controls::SensorBlackLevels); >>> if (blackLevels) { >>> - Span<const int32_t> levels = *blackLevels; >>> + Span<const int32_t, 4> levels = *blackLevels; >>> >>> /* >>> * The black levels control is specified in R, Gr, Gb, B order. >>> diff --git a/utils/gen-controls.py b/utils/gen-controls.py >>> index 3f99b5e2..46ba4394 100755 >>> --- a/utils/gen-controls.py >>> +++ b/utils/gen-controls.py >>> @@ -7,6 +7,8 @@ >>> # gen-controls.py - Generate control definitions from YAML >>> >>> import argparse >>> +from functools import reduce >>> +import operator >>> import string >>> import sys >>> import yaml >>> @@ -22,6 +24,24 @@ def format_description(description): >>> return '\n'.join([(line and ' * ' or ' *') + line for line in description]) >>> >>> >>> +def get_ctrl_type(ctrl): >>> + ctrl_type = ctrl['type'] >>> + ctrl_size_arr = ctrl.get('size') >>> + >>> + if ctrl_type == 'string': >>> + return 'std::string' >>> + elif ctrl_size_arr is not None: >>> + if len(ctrl_size_arr) > 0: >>> + # fixed-sized Span >>> + ctrl_span_size = reduce(operator.mul, ctrl_size_arr) >>> + return f"Span<const {ctrl_type}, {ctrl_span_size}>" >>> + else: >>> + # variable-sized Span >>> + return f"Span<const {ctrl_type}>" >>> + else: >>> + return ctrl_type >>> + >>> + >>> def generate_cpp(controls): >>> enum_doc_start_template = string.Template('''/** >>> * \\enum ${name}Enum >>> @@ -50,11 +70,7 @@ ${description} >>> name, ctrl = ctrl.popitem() >>> id_name = snake_case(name).upper() >>> >>> - ctrl_type = ctrl['type'] >>> - if ctrl_type == 'string': >>> - ctrl_type = 'std::string' >>> - elif ctrl.get('size'): >>> - ctrl_type = 'Span<const %s>' % ctrl_type >>> + ctrl_type = get_ctrl_type(ctrl) >>> >>> info = { >>> 'name': name, >>> @@ -135,11 +151,7 @@ def generate_h(controls): >>> >>> ids.append('\t' + id_name + ' = ' + str(id_value) + ',') >>> >>> - ctrl_type = ctrl['type'] >>> - if ctrl_type == 'string': >>> - ctrl_type = 'std::string' >>> - elif ctrl.get('size'): >>> - ctrl_type = 'Span<const %s>' % ctrl_type >>> + ctrl_type = get_ctrl_type(ctrl) >>> >>> info = { >>> 'name': name, >>> -- >>> 2.34.1 >>>
On Wed, Aug 03, 2022 at 11:44:01AM +0200, Jacopo Mondi via libcamera-devel wrote: > Hi Christian, > > On Tue, Aug 02, 2022 at 11:03:24PM +0200, Christian Rauch wrote: > > Define Span types explicitly as either variable- or fixed-sized. This > > introduces a new convention for defining Span dimensions in the property > > and control value definitions and generates Span types as variable-sized > > Span<T> or as fixed-sized Span<T,N>. > > > > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > The issue with gcc8 seems now solved. > > One more tag and I hope we can collect the patch ? There are still a few things I'd like to change, but I'll do so by submitting patches on top, it will be more efficient. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/controls.h | 2 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 19 +++++++++-------- > > src/libcamera/control_ids.yaml | 4 ++-- > > src/libcamera/property_ids.yaml | 4 ++-- > > src/qcam/dng_writer.cpp | 2 +- > > utils/gen-controls.py | 32 ++++++++++++++++++++--------- > > 6 files changed, 38 insertions(+), 25 deletions(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index b1b52acb..ebc168fc 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -168,7 +168,7 @@ public: > > > > using V = typename T::value_type; > > const V *value = reinterpret_cast<const V *>(data().data()); > > - return { value, numElements_ }; > > + return T{ value, numElements_ }; > > } > > > > #ifndef __DOXYGEN__ > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index 6befdd71..69c73f8c 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -567,18 +567,19 @@ void IPARPi::reportMetadata() > > > > AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status"); > > if (awbStatus) { > > - libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gainR), > > - static_cast<float>(awbStatus->gainB) }); > > + libcameraMetadata_.set(controls::ColourGains, > > + Span<const float, 2>({ static_cast<float>(awbStatus->gainR), > > + static_cast<float>(awbStatus->gainB) })); > > libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK); > > } > > > > BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status"); > > if (blackLevelStatus) > > libcameraMetadata_.set(controls::SensorBlackLevels, > > - { static_cast<int32_t>(blackLevelStatus->blackLevelR), > > - static_cast<int32_t>(blackLevelStatus->blackLevelG), > > - static_cast<int32_t>(blackLevelStatus->blackLevelG), > > - static_cast<int32_t>(blackLevelStatus->blackLevelB) }); > > + Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR), > > + static_cast<int32_t>(blackLevelStatus->blackLevelG), > > + static_cast<int32_t>(blackLevelStatus->blackLevelG), > > + static_cast<int32_t>(blackLevelStatus->blackLevelB) })); > > > > FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status"); > > if (focusStatus && focusStatus->num == 12) { > > @@ -883,7 +884,7 @@ void IPARPi::queueRequest(const ControlList &controls) > > if (gains[0] != 0.0f && gains[1] != 0.0f) > > /* A gain of 0.0f will switch back to auto mode. */ > > libcameraMetadata_.set(controls::ColourGains, > > - { gains[0], gains[1] }); > > + Span<const float, 2>({ gains[0], gains[1] })); > > break; > > } > > > > @@ -1167,8 +1168,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur > > > > /* Return the validated limits via metadata. */ > > libcameraMetadata_.set(controls::FrameDurationLimits, > > - { static_cast<int64_t>(minFrameDuration_.get<std::micro>()), > > - static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }); > > + Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()), > > + static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) })); > > > > /* > > * Calculate the maximum exposure time possible for the AGC to use. > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml > > index ecab3ae9..5fa168c6 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -291,7 +291,7 @@ controls: > > transformation. The 3x3 matrix is stored in conventional reading > > order in an array of 9 floating point values. > > > > - size: [3x3] > > + size: [3,3] > > > > - ScalerCrop: > > type: Rectangle > > @@ -525,7 +525,7 @@ controls: > > the window where the focal distance for the objects shown in that part > > of the image are closest to the camera. > > > > - size: [n] > > + size: [] > > > > - AfTrigger: > > type: int32_t > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > > index 11b7ebdc..a87485d7 100644 > > --- a/src/libcamera/property_ids.yaml > > +++ b/src/libcamera/property_ids.yaml > > @@ -497,7 +497,7 @@ controls: > > > > - PixelArrayOpticalBlackRectangles: > > type: Rectangle > > - size: [n] > > + size: [] > > description: | > > The pixel array region(s) which contain optical black pixels > > considered valid for calibration purposes. > > @@ -592,7 +592,7 @@ controls: > > > > - PixelArrayActiveAreas: > > type: Rectangle > > - size: [n] > > + size: [] > > description: | > > The PixelArrayActiveAreas property defines the (possibly multiple and > > overlapping) portions of the camera sensor readable pixel matrix > > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp > > index 5b94b3f2..b4362537 100644 > > --- a/src/qcam/dng_writer.cpp > > +++ b/src/qcam/dng_writer.cpp > > @@ -517,7 +517,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > > > const auto &blackLevels = metadata.get(controls::SensorBlackLevels); > > if (blackLevels) { > > - Span<const int32_t> levels = *blackLevels; > > + Span<const int32_t, 4> levels = *blackLevels; > > > > /* > > * The black levels control is specified in R, Gr, Gb, B order. > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py > > index 3f99b5e2..46ba4394 100755 > > --- a/utils/gen-controls.py > > +++ b/utils/gen-controls.py > > @@ -7,6 +7,8 @@ > > # gen-controls.py - Generate control definitions from YAML > > > > import argparse > > +from functools import reduce > > +import operator > > import string > > import sys > > import yaml > > @@ -22,6 +24,24 @@ def format_description(description): > > return '\n'.join([(line and ' * ' or ' *') + line for line in description]) > > > > > > +def get_ctrl_type(ctrl): > > + ctrl_type = ctrl['type'] > > + ctrl_size_arr = ctrl.get('size') > > + > > + if ctrl_type == 'string': > > + return 'std::string' > > + elif ctrl_size_arr is not None: > > + if len(ctrl_size_arr) > 0: > > + # fixed-sized Span > > + ctrl_span_size = reduce(operator.mul, ctrl_size_arr) > > + return f"Span<const {ctrl_type}, {ctrl_span_size}>" > > + else: > > + # variable-sized Span > > + return f"Span<const {ctrl_type}>" > > + else: > > + return ctrl_type > > + > > + > > def generate_cpp(controls): > > enum_doc_start_template = string.Template('''/** > > * \\enum ${name}Enum > > @@ -50,11 +70,7 @@ ${description} > > name, ctrl = ctrl.popitem() > > id_name = snake_case(name).upper() > > > > - ctrl_type = ctrl['type'] > > - if ctrl_type == 'string': > > - ctrl_type = 'std::string' > > - elif ctrl.get('size'): > > - ctrl_type = 'Span<const %s>' % ctrl_type > > + ctrl_type = get_ctrl_type(ctrl) > > > > info = { > > 'name': name, > > @@ -135,11 +151,7 @@ def generate_h(controls): > > > > ids.append('\t' + id_name + ' = ' + str(id_value) + ',') > > > > - ctrl_type = ctrl['type'] > > - if ctrl_type == 'string': > > - ctrl_type = 'std::string' > > - elif ctrl.get('size'): > > - ctrl_type = 'Span<const %s>' % ctrl_type > > + ctrl_type = get_ctrl_type(ctrl) > > > > info = { > > 'name': name,
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index b1b52acb..ebc168fc 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -168,7 +168,7 @@ public: using V = typename T::value_type; const V *value = reinterpret_cast<const V *>(data().data()); - return { value, numElements_ }; + return T{ value, numElements_ }; } #ifndef __DOXYGEN__ diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 6befdd71..69c73f8c 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -567,18 +567,19 @@ void IPARPi::reportMetadata() AwbStatus *awbStatus = rpiMetadata_.getLocked<AwbStatus>("awb.status"); if (awbStatus) { - libcameraMetadata_.set(controls::ColourGains, { static_cast<float>(awbStatus->gainR), - static_cast<float>(awbStatus->gainB) }); + libcameraMetadata_.set(controls::ColourGains, + Span<const float, 2>({ static_cast<float>(awbStatus->gainR), + static_cast<float>(awbStatus->gainB) })); libcameraMetadata_.set(controls::ColourTemperature, awbStatus->temperatureK); } BlackLevelStatus *blackLevelStatus = rpiMetadata_.getLocked<BlackLevelStatus>("black_level.status"); if (blackLevelStatus) libcameraMetadata_.set(controls::SensorBlackLevels, - { static_cast<int32_t>(blackLevelStatus->blackLevelR), - static_cast<int32_t>(blackLevelStatus->blackLevelG), - static_cast<int32_t>(blackLevelStatus->blackLevelG), - static_cast<int32_t>(blackLevelStatus->blackLevelB) }); + Span<const int32_t, 4>({ static_cast<int32_t>(blackLevelStatus->blackLevelR), + static_cast<int32_t>(blackLevelStatus->blackLevelG), + static_cast<int32_t>(blackLevelStatus->blackLevelG), + static_cast<int32_t>(blackLevelStatus->blackLevelB) })); FocusStatus *focusStatus = rpiMetadata_.getLocked<FocusStatus>("focus.status"); if (focusStatus && focusStatus->num == 12) { @@ -883,7 +884,7 @@ void IPARPi::queueRequest(const ControlList &controls) if (gains[0] != 0.0f && gains[1] != 0.0f) /* A gain of 0.0f will switch back to auto mode. */ libcameraMetadata_.set(controls::ColourGains, - { gains[0], gains[1] }); + Span<const float, 2>({ gains[0], gains[1] })); break; } @@ -1167,8 +1168,8 @@ void IPARPi::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDur /* Return the validated limits via metadata. */ libcameraMetadata_.set(controls::FrameDurationLimits, - { static_cast<int64_t>(minFrameDuration_.get<std::micro>()), - static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) }); + Span<const int64_t, 2>({ static_cast<int64_t>(minFrameDuration_.get<std::micro>()), + static_cast<int64_t>(maxFrameDuration_.get<std::micro>()) })); /* * Calculate the maximum exposure time possible for the AGC to use. diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml index ecab3ae9..5fa168c6 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -291,7 +291,7 @@ controls: transformation. The 3x3 matrix is stored in conventional reading order in an array of 9 floating point values. - size: [3x3] + size: [3,3] - ScalerCrop: type: Rectangle @@ -525,7 +525,7 @@ controls: the window where the focal distance for the objects shown in that part of the image are closest to the camera. - size: [n] + size: [] - AfTrigger: type: int32_t diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml index 11b7ebdc..a87485d7 100644 --- a/src/libcamera/property_ids.yaml +++ b/src/libcamera/property_ids.yaml @@ -497,7 +497,7 @@ controls: - PixelArrayOpticalBlackRectangles: type: Rectangle - size: [n] + size: [] description: | The pixel array region(s) which contain optical black pixels considered valid for calibration purposes. @@ -592,7 +592,7 @@ controls: - PixelArrayActiveAreas: type: Rectangle - size: [n] + size: [] description: | The PixelArrayActiveAreas property defines the (possibly multiple and overlapping) portions of the camera sensor readable pixel matrix diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp index 5b94b3f2..b4362537 100644 --- a/src/qcam/dng_writer.cpp +++ b/src/qcam/dng_writer.cpp @@ -517,7 +517,7 @@ int DNGWriter::write(const char *filename, const Camera *camera, const auto &blackLevels = metadata.get(controls::SensorBlackLevels); if (blackLevels) { - Span<const int32_t> levels = *blackLevels; + Span<const int32_t, 4> levels = *blackLevels; /* * The black levels control is specified in R, Gr, Gb, B order. diff --git a/utils/gen-controls.py b/utils/gen-controls.py index 3f99b5e2..46ba4394 100755 --- a/utils/gen-controls.py +++ b/utils/gen-controls.py @@ -7,6 +7,8 @@ # gen-controls.py - Generate control definitions from YAML import argparse +from functools import reduce +import operator import string import sys import yaml @@ -22,6 +24,24 @@ def format_description(description): return '\n'.join([(line and ' * ' or ' *') + line for line in description]) +def get_ctrl_type(ctrl): + ctrl_type = ctrl['type'] + ctrl_size_arr = ctrl.get('size') + + if ctrl_type == 'string': + return 'std::string' + elif ctrl_size_arr is not None: + if len(ctrl_size_arr) > 0: + # fixed-sized Span + ctrl_span_size = reduce(operator.mul, ctrl_size_arr) + return f"Span<const {ctrl_type}, {ctrl_span_size}>" + else: + # variable-sized Span + return f"Span<const {ctrl_type}>" + else: + return ctrl_type + + def generate_cpp(controls): enum_doc_start_template = string.Template('''/** * \\enum ${name}Enum @@ -50,11 +70,7 @@ ${description} name, ctrl = ctrl.popitem() id_name = snake_case(name).upper() - ctrl_type = ctrl['type'] - if ctrl_type == 'string': - ctrl_type = 'std::string' - elif ctrl.get('size'): - ctrl_type = 'Span<const %s>' % ctrl_type + ctrl_type = get_ctrl_type(ctrl) info = { 'name': name, @@ -135,11 +151,7 @@ def generate_h(controls): ids.append('\t' + id_name + ' = ' + str(id_value) + ',') - ctrl_type = ctrl['type'] - if ctrl_type == 'string': - ctrl_type = 'std::string' - elif ctrl.get('size'): - ctrl_type = 'Span<const %s>' % ctrl_type + ctrl_type = get_ctrl_type(ctrl) info = { 'name': name,