[libcamera-devel] [PATCH 3/5] provide a default fixed-sized Span constructor
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Apr 4 13:32:29 CEST 2022
Hello,
On Mon, Apr 04, 2022 at 12:32:47PM +0200, Jacopo Mondi wrote:
> 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:
> > > On Sat, Apr 02, 2022 at 12:05:43AM +0100, Christian Rauch wrote:
> > >> Am 01.04.22 um 15:28 schrieb Jacopo Mondi:
> > >>> 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>{});
I don't think this will do what you expect. The code is equivalent to
std::array<value_type, extent> data{};
Span(data);
The first line allocates an array of extent default-initialized elements
on the stack. The second line calls the Span constructor that takes an
std::array argument, and the span's data_ pointer points to
array.data(). Then, when you return from the destructor, the array
variable is destroyed, and data_ points to freed memory.
The Span class is modelled after std::span, introduced in C++20. When
libcamera will move to C++20 (I don't expect that to happen before a few
years), the plan is to switch to std::span. We should thus keep the Span
implementation as close as possible to std::span (it can't be exactly
identical, as std::span relies on other C++20 features, for instance
constructor 7 in [1] uses the C++20 range API, so we have two different
constructors that take a reference to a container instead).
[1] https://en.cppreference.com/w/cpp/container/span/span
> > >>>> + }
> > >>>> +
> > >>>
> > >>> 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.
I do like std::optional, it's a useful feature. We use it internally,
and we can extend its usage. However, while libcamera is compiled for
C++17, we keep the public API C++14-compatible. This was decided to
allow applications still using C++14 to compile and link against
libcamera, with the main use case being Chromium (for which we have
implement native libcamera support in [2], not merged in mainline yet).
It seems that Chromium has now moved to C++17 ([3]), so we could
possibly follow suit. We'll have to rebuild and test Chromium
integration on the latest version first.
This being said, I would prefer keeping the public API C++14-compatible
for a bit longer if possible, as not all applications have moved to
C++17. If C++17 feature would be so useful in the public API that
dropping support for C++14 users would be an acceptable drawback, I'm
not against that
[2] https://github.com/libcamera-org/chromium
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=752720
> 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".
> >
> > >>> Span<int, 4> s{}
> > >>> if (!s.empty())
> > >>> error!
> > >>>
> > >>>> /* Span<int, 4>{}; */
> > >>>>
> > >>>> Span<int, 4>{ &i[0], 4 };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list