[libcamera-devel] [PATCH 3/5] provide a default fixed-sized Span constructor
Christian Rauch
Rauch.Christian at gmx.de
Tue Apr 5 02:46:31 CEST 2022
Dear Laurent,
Thanks for pointing out that the "std::array" in my patch is only
temporarily constructed. I overlooked that the Span only stores the
pointer to the data and the size, and not the actual data.
This, and other minor issues, have been fixed in version 2 of my patch set.
Best,
Christian
Am 04.04.22 um 12:32 schrieb Laurent Pinchart:
> 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 };
>
More information about the libcamera-devel
mailing list