[libcamera-devel] [PATCH] cam: convert to libfmt

[Kieran Bingham]

Quoting Laurent Pinchart (2022-05-10 14:04:57)
> Hi Eric,
> 
> 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:
> > > >
> > > > > This is just a conversation starter, not for merging. I really like
> > > > > libfmt, and I really hate the C++ stream-based string formatting. libfmt
> > > > > is the first library I add to any new C++ project I make.
> > > > >
> > > > > You can find more information about libfmt from:
> > > > >
> > > > > https://github.com/fmtlib/fmt
> > > > > https://fmt.dev/latest/index.html
> > > > >
> > > > > This patch is just a crude conversion with ugly macros to showcase what
> > > > > the formatting code might look like.
> > > > >
> > > > > libfmt pages suggest that libfmt would be faster and smaller (exe size)
> > > > > than iostreams, but for the size it didn't seem to be true in cam's case
> > > > > as the tests below show. However, simple prints did reduce the exe size,
> > > > > but the few more complex ones increased the size.
> > > > >
> > > > > Size tests with gcc 11.2.0-19ubuntu1
> > > > >
> > > > > - Without libfmt
> > > > >
> > > > > debug           3523400
> > > > > debug lto       3269368
> > > > > release         223056
> > > > > release lto     172280
> > > > >
> > > > > - With libfmt
> > > > >
> > > > > debug           4424256
> > > > > debug lto       4143840
> > > > > release         303952
> > > > > release lto     252640
> > > > >
> > > > > Above shows that cam's size clearly increases with libfmt. However, the
> > > > > increase really comes only from one case, the use of fmt::memory_buffer
> > > > > and std::back_inserter. Converting that code to use fmt::format() and
> > > > > naive string append gives:
> > > > >
> > > > > release with string append      233680
> > > > > release lto with string append  186936
> > > > >
> > > > > Also, if I add another use of fmt::memory_buffer and std::back_inserter
> > > > > to another file, I see much less increase in the size:
> > > > >
> > > > > release lto with two uses of memory_buffer, back_inserter       256736
> > > > >
> > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> > > >
> > > > 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 ..


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.

Maybe this deserves it's own thread...

--
Kieran


> 
> > 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.
> 
> -- 
> Regards,
> 
> Laurent Pinchart