[libcamera-devel] Usage of C++17 in the public API (was "[PATCH] cam: convert to libfmt")

[Christian Rauch]

Am 30.05.22 um 10:03 schrieb Laurent Pinchart:
> On Mon, May 30, 2022 at 09:47:25AM +0100, Eric Curtin via libcamera-devel wrote:
>> On Sat, 28 May 2022 at 00:19, Christian Rauch wrote:
>>> Am 22.05.22 um 11:52 schrieb Laurent Pinchart via libcamera-devel:
>>>> On Sun, May 22, 2022 at 11:08:27AM +0100, Kieran Bingham wrote:
>>>>> Quoting Laurent Pinchart (2022-05-10 14:04:57)
>>>>>> On Tue, May 10, 2022 at 01:59:37PM +0100, Eric Curtin via libcamera-devel wrote:
>>>>>>> On Tue, 10 May 2022 at 11:10, Kieran Bingham via libcamera-devel wrote:
>>>>>>>> Quoting Naushir Patuck (2022-05-10 10:49:14)
>>>>>>>>> On Tue, 10 May 2022 at 08:16, Tomi Valkeinen via libcamera-devel wrote:
>>>>
>>>> [snip]
>>>>
>>>>>>>>> For what it's worth, I absolutely loathe formatting in std iostream, and
>>>>>>>>> libfmt is a wonderful alternative.
>>>>>>>>> It also is close enough to the C++20 std::format implementation that
>>>>>>>>> eventual porting would be low effort.  So I am all for this change :)
>>>>>>
>>>>>> It's going to take a while before we can switch to C++20, but I'm
>>>>>> looking forward to it (and before that to some of the C++17 features
>>>>>> too, it would be nice to use std::optional in the public API).
>>>>>
>>>>> So, I've just seen as Christian said - we already do!
>>>>>
>>>>> It's used by color space ..
>>>>
>>>> Oops...
>>>>
>>>>> So we're faced with either reverting, or rolling back on the color-space
>>>>> change, or pushing forwards with C++17 on the public-api.
>>>>
>>>> Switching to C++17 worries me as it's fairly recent and may not be
>>>> easily available for all users. std::optional could easily rbe replaced
>>>> with a custom implementation in utils::optional if needed. I'm tempted
>>>> to leave std::optional in for now, and accept new users of std::optional
>>>> but no other C++17 APIs. Switching to utils::optional for ColorSpace
>>>> would cover new users of std::optional so we're not creating any new
>>>> liability.
>>>
>>> I think that C++17 is quite old by now (nearly 5 years) and all major
>>> compilers support it for multiple versions and years now. Do you know of
>>> an example, where requiring C++17 would hinder the adaptation of
>>> libcamera? Do any of the target system not support this standard?
>
> The main blocker used to tbe Chromium, which used C++14. It seems to
> have moved to C++17 now, and we have also dropped the plan for direct
> integration of libcamera support in Chromium (and Firefox) as they
> should go through PipeWire and the XDG camera portal.
>
>>> I think the use of std::optional to express invalid values is quite
>>> significant compared to the previous solution of using default
>>> constructed values. Reimplementing this in libcamera would be a
>>> solution, but it just adds development and maintenance costs for no
>>> obvious reason (to me).
>
> We can use std::optional (as it's there already). *If* C++17 support
> ends up being an issue in the future, requiring us to move back to
> C++14, then it will be easy to make a custom implementation, but only if
> necessary (we've done that with std::span already, which is only
> available in C++20).
>
>>>>> Maybe this deserves it's own thread...
>>>>
>>>> Done :-)
>>>>
>>>>>>> I am all in for this change also, personally I would have changed to
>>>>>>> printf for now
>>>>>>> to have one less dependency (also an easy port to C++20 std::format). The
>>>>>>> dependency list is getting large, I noticed a build of mine failing
>>>>>>> recently because
>>>>>>> I didn't have libyaml.
>>>>>>
>>>>>> I've posted a patch that falls back to a subproject in that case. Would
>>>>>> you be able to give it a try ?
>>>>>>
>>>>>>> But std::format and libfmt are quite fast and anything is better than
>>>>>>> streams so +1.
>>>>>>>
>>>>>>>> I've never used (yet) {fmt}, but I've only heard good things about
>>>>>>>> performance, and of course it's headed into the standard, so I also
>>>>>>>> think there is some good merit to be found in this development.
>>
>> Only speaking for the red hat distros, every distro that has a package
>> built for libcamera or could have in future has at least C++17,
>> including newly released LTS release RHEL9.
>
> That's good to know. The other question is whether all C++ applications
> that may want to use libcamera can be compiled with C++17 or newer.

Isn't C++17 backwards compatible? My understanding is that every earlier
C++ standard (e.g. C++11) should be able to compile with a C++17 (or
later) compiler.

>
>> Given libcamera is still 0.0 it could be a nice opportunity to take.
>