[libcamera-devel] [PATCH 3/5] provide a default fixed-sized Span constructor
Jacopo Mondi
jacopo at jmondi.org
Mon Apr 4 12:32:47 CEST 2022
Hi Christian
+ Laurent
On Sat, Apr 02, 2022 at 12:28:05PM +0100, Christian Rauch wrote:
> Hi Jacopo,
>
> See my response below, specifically see the case about "std::optional".
>
> Am 02.04.22 um 10:46 schrieb Jacopo Mondi:
> > Hi Christian,
> >
> > On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote:
> >> Hi Jacopo,
> >>
> >> Thanks for reviewing this. See my responses inline below.
> >>
> >>
> >> Am 01.04.22 um 15:28 schrieb Jacopo Mondi:
> >>> Hi Christian
> >>>
> >>> On Fri, Apr 01, 2022 at 01:06:14AM +0100, Christian Rauch via libcamera-devel wrote:
> >>>> The new default constructor allows to construct a fixed-sized Span via the
> >>>> default constructor of its stored data type.
> >>>> This prevents the construction of empty fixed-sized Spans that cannot hold
> >>>> any data.
> >>>>
> >>>> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
> >>>> ---
> >>>> include/libcamera/base/span.h | 5 +++++
> >>>> include/libcamera/controls.h | 2 +-
> >>>> test/span.cpp | 2 +-
> >>>> 3 files changed, 7 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h
> >>>> index 88d2e3de..7a4806dc 100644
> >>>> --- a/include/libcamera/base/span.h
> >>>> +++ b/include/libcamera/base/span.h
> >>>> @@ -112,6 +112,11 @@ public:
> >>>> {
> >>>> }
> >>>>
> >>>> + Span()
> >>>> + {
> >>>> + Span(std::array<value_type, extent>{});
> >>>> + }
> >>>> +
> >>>
> >>> This constructor creates a span of 'extent' elements all of them
> >>> initialized to 0 then ?
> >>
> >> All elements will be default constructed. Integers and floats will be
> >> initialised to 0, "std::strings" will be empty, and 'Size' and
> >> 'Rectangle' will use their specified default constructors.
> >> The default initialisation is required because the fixed-sized array
> >> cannot be empty.
> >>
> >
> > You're right, "initialized to 0" is not correct, "default constructed"
> > is the term I should have used :)
> >
> >>>
> >>> If I remove it I get
> >>>
> >>> ../include/libcamera/controls.h:380:34: error: no matching function for call to ‘libcamera::Span<const float, 2>::Span(<brace-enclosed initializer list>)’
> >>> 380 | return T{};
> >>>
> >>> Caused by
> >>> template<typename T>
> >>> T get(const Control<T> &ctrl) const
> >>> {
> >>> const ControlValue *val = find(ctrl.id());
> >>> if (!val)
> >>> return T{}; <------
> >>>
> >>> return val->get<T>();
> >>> }
> >>>
> >>> as now that extent is != 0 the expression doesn't bind anymore to the
> >>> existing constructor:
> >>>
> >>> template<bool Dependent = false,
> >>> typename = std::enable_if_t<Dependent || Extent == 0>>
> >>> constexpr Span() noexcept
> >>> : data_(nullptr)
> >>> {
> >>> }
> >>>
> >>> (I don't fully get what is the use of Dependent in the above definition :)
> >>
> >> I also do not understand the use of this constructor, since a
> >> 0-fixed-size Span cannot store anything.
> >
> > I think it was used to identify a non-valid span, to be returned in
> > case of errors
> >
> >>>
> >>> However I feel like creating a Span of Extent elements all zeroed is a
> >>> bit hill-defined, considering it is only used to return an error
> >>> condition. In example, if I'm not mistaken a Span constructed with
> >>> your proposed constructor will
> >>>
> >>> constexpr size_type size() const noexcept { return Extent; }
> >>> constexpr size_type size_bytes() const noexcept { return size() * sizeof(element_type); }
> >>> constexpr bool empty() const noexcept { return size() == 0; }
> >>>
> >>> be !empty and of size == 2, which is misleading considering we really
> >>> want to return an empty Span in case of error and the caller should
> >>> not be mis-leaded thinking it is valid.
> >>>
> >>> I don't have any super smart proposal to offer, if not recording the
> >>> 'valid' or 'empty' state in a class member and use it in size() and
> >>> empty() instead of relying on Extent which is defined at overload
> >>> resolution time and cannot be assigned at run time.
> >>
> >> I think this is a general issue with return default constructed objects
> >> to indicate an error. The error is only defined implicitly. If
> >> ControlList::get returns a 'Size' with width=0 and height=0, then it is
> >> not clear if this indicates an error or if this value is really stored.
> >
> > Yes and no, if the custom type has a well defined interface it can be
> > provided with an isValid()/isNull() function, like it is done for the
> > Size class
> >
> > bool isNull() const { return !width && !height; }
> >
> > I understand that
> >
> > Size s = controlList.get(SomeSizeControl);
> > if (s.isNull()) {
> > /* Error out */
> > }
> >
> > Requires knowledge of the custom type interface and special care as
> > it's less intuitive than handling an int return code or a pointer, but
> > either we forbid return-by-value function signatures completely or we
> > have to instrument custom types with an isValid() function for the
> > caller to test.
>
> But this will only ever work for custom types. Basic C types and C++
> standard types do not have such a flag. "isNull" only means that the
> area of the Size is zero, but it does not directly mean that the
> requested "Size" does not exist.
>
To me standard type are less a concern as they all have functions to test if
they're empty. Of course this assumes an empty vector returned by a
function is an error condition, something that it's not semantically
accurate, I agree
> >
> > The Span class validity test is based on its Extent size, and the
> > constructor you have introduced makes an invalid Span look like it is
> > valid.
>
> I really don't like this idea of implicitly encoding "validity" by a
> specific value :-)
>
> >
> >>
> >> You could add special class members to indicate that an object is
> >> "valid", but this is not a nice solution and only works for custom types
> >
> > Other custom libcamera types are fine in that regard. I was suggesting
> > to add a 'bool valid_;' class member to Span, to not rely on Extent to
> > test its validity.
> >
> >> in libcamera. The "C" way would probably to indicate an error by a NULL
> >> pointer and a returned error number.
> >>
> >> I think the proper way would be to throw an exception. This could also
> >> be a custom libcamera exception.
> >>
> >
> > Too bad libcamera doesn't use exceptions ;)
>
> Why is that? Has this something to do with "embedded C++"? Alternatively
I think among the reasons, apart from code size and efficiency on
which I don't have numbers to debate on but seems sensible for some
embedded platforms, there is the fact that using exceptions
would require code that use libcamera to be exception-safe, it would
have made more complex creating a C-API and deal with errors through
IPC (which libcamera use heavily to communicate with IPA modules).
> to exceptions, the get method could either 1) take a target value by
> reference and return a status flag (success / failure) or 2) take a
> status flag by reference and return a default constructed object.
>
> There is also "std::optional<>" which can express this: "A common use
> case for optional is the return value of a function that may fail."
>
> If you are ok with newer C++ features, than "optional" is maybe the most
> "expressive" solution:
>
> std::optional<Size> ret = controlList.get(SomeSizeControl);
> if (ret.has_value()) {
> // OK
> Size s = s.value();
> }
> else {
> // ERROR
> }
>
> What do you think about this? The "optional" is essentially attaching a
> validity flag to any object returned by the "get" method, but it does
> not require changing the type itself.
>
std::optional<> has been introduced in C++17, which libcamera migrated
to only 1.5 year ago. At the time the Control interface had been
designed std::optional<> was not available, if memory does not fail me.
However it seems a rather neat way to handle return-by-value error
conditions and I would be in favour to move the code base to use it.
We already have a user in libcamera, introduced by RPi when adding
support to color space handling
include/libcamera/internal/v4l2_device.h: static std::optional<ColorSpace> toColorSpace(const T &v4l2Format);
Let me rope-in Laurent by CC-ing him to know what he thinks.
Thanks for the discussion starter!
> >
> >>>
> >>>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)
> >>>> : data_(ptr)
> >>>> {
> >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >>>> index 665bcac1..de8a7770 100644
> >>>> --- a/include/libcamera/controls.h
> >>>> +++ b/include/libcamera/controls.h
> >>>> @@ -167,7 +167,7 @@ public:
> >>>>
> >>>> using V = typename T::value_type;
> >>>> const V *value = reinterpret_cast<const V *>(data().data());
> >>>> - return { value, numElements_ };
> >>>> + return T{ value, numElements_ };
> >>>
> >>> this applies to overload
> >>>
> >>> template<typename T, typename std::enable_if_t<details::is_span<T>::value ||
> >>> std::is_same<std::string, std::remove_cv_t<T>>::value,
> >>> T get() const
> >>> {
> >>> assert(type_ == details::control_type<std::remove_cv_t<T>>::value);
> >>> assert(isArray_);
> >>>
> >>> using V = typename T::value_type;
> >>> const V *value = reinterpret_cast<const V *>(data().data());
> >>> return { value, numElements_ };
> >>> }
> >>>
> >>> Isn't the returned { value, numElements_ } already used to construct an
> >>> instance of T if assigned to an lvalue expression ?
> >>>
> >>> IOW why do you need to contruct an instance of T and return it
> >>> explicitely ? Thanks to return value optimization this shouldn't have
> >>> any performance impact but I have missed why it is needed.
> >>>
> >>> <2 minutes later>
> >>>
> >>> And now that I wrote this, if I remove it I get
> >>>
> >>> include/libcamera/controls.h:170:46: error: converting to
> >>> ‘libcamera::Span<const float, 2>’ from initializer list would use
> >>> explicit constructor ‘constexpr libcamera::Span<T,
> >>> Extent>::Span(libcamera::Span<T, Extent>::pointer, libcamera::Span<T,
> >>> Extent>::size_type) [with T = const float; long unsigned int Extent =
> >>> 2; libcamera::Span<T, Extent>::pointer = const float*;
> >>> libcamera::Span<T, Extent>::size_type = long unsigned int]’
> >>>
> >>> Because indeed
> >>> explicit constexpr Span(pointer ptr, [[maybe_unused]] size_type count)
> >>>
> >>> is, well, explicit (which I think it's ok and should not be removed)
> >>>
> >>> I'll leave here the above text anyway for others that might have the
> >>> same question :)
> >>>
> >>>> }
> >>>>
> >>>> #ifndef __DOXYGEN__
> >>>> diff --git a/test/span.cpp b/test/span.cpp
> >>>> index abf3a5d6..c37e2a66 100644
> >>>> --- a/test/span.cpp
> >>>> +++ b/test/span.cpp
> >>>> @@ -37,7 +37,7 @@ protected:
> >>>> * to generate undefined behaviour.
> >>>> */
> >>>>
> >>>> - Span<int, 0>{};
> >>>> + /* Span<int, 0>{}; */
> >>>
> >>> You should remove it, or if the above issue with constructing an
> >>> 'empty' Span of size > 0 is resolved, prove that it works as intended.
> >>
> >> I commented it out instead of removing this, because the comment above
> >> says "[...] Commented-out tests are expected not to compile [...]" and
> >> there was already a commented case for an 4-sized Span. Anyway, I don't
> >
> > Oh I see, sorry, missed it.
> >
> >> see a real application of a 0-fixed-sized Span, as it will never be able
> >> to store anything.
> >
> > My understanding is that it is only used for error conditions.
>
> In this case, this "error condition" will be replaced by another
> compiler error that will trigger for the same reasons (empty span), but
> will be triggered by trying to construct an empty "std::array".
>
> Best,
> Christian
>
> >
> > Thanks
> > j
> >
> >>
> >> Best,
> >> Christian
> >>
> >>
> >>>
> >>> Span<int, 4> s{}
> >>> if (!s.empty())
> >>> error!
> >>>
> >>> Thanks
> >>> j
> >>>
> >>>> /* Span<int, 4>{}; */
> >>>>
> >>>> Span<int, 4>{ &i[0], 4 };
> >>>> --
> >>>> 2.25.1
> >>>>
More information about the libcamera-devel
mailing list