[libcamera-devel] cam: convert to libfmt

Message ID	20220510071617.42227-1-tomi.valkeinen@ideasonboard.com
State	New
Headers	show Return-Path: <libcamera-devel-bounces@lists.libcamera.org> To: libcamera-devel@lists.libcamera.org, Kieran Bingham <kieran.bingham@ideasonboard.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com> Date: Tue, 10 May 2022 10:16:17 +0300 Message-Id: <20220510071617.42227-1-tomi.valkeinen@ideasonboard.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [libcamera-devel] [PATCH] cam: convert to libfmt Precedence: list From: Tomi Valkeinen via libcamera-devel <libcamera-devel@lists.libcamera.org> Reply-To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" <libcamera-devel-bounces@lists.libcamera.org>
Series	[libcamera-devel] cam: convert to libfmt
Related	show [libcamera-devel] cam: convert to libfmt

Tomi Valkeinen May 10, 2022, 7:16 a.m. UTC

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@ideasonboard.com>
---
 src/cam/camera_session.cpp | 105 ++++++++++++++++---------------------
 src/cam/drm.cpp            |  68 ++++++++----------------
 src/cam/event_loop.cpp     |   9 ++--
 src/cam/file_sink.cpp      |  31 +++++------
 src/cam/image.cpp          |  15 +++---
 src/cam/kms_sink.cpp       |  38 ++++++--------
 src/cam/main.cpp           |  28 +++++-----
 src/cam/meson.build        |   5 +-
 src/cam/options.cpp        |  66 ++++++++++-------------
 src/cam/stream_options.cpp |  18 +++----
 10 files changed, 165 insertions(+), 218 deletions(-)

Tomi Valkeinen May 10, 2022, 9:10 a.m. UTC | #1

On 10/05/2022 10:16, Tomi Valkeinen 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.

As for the PR and EPR macros, those was just something to get forward 
with. I think fmt::print("") is fine, but fmt::print(stderr, "") is a 
bit long.

Compared to cout, "fmt::print(" is actually shorter than "std::cout << 
", and without all those << and std::endl the lines are shorter.

Not so with cerr and fmt::print(stderr, (although in many cases the 
total length would still be shorter) but I think it makes sense to have 
a macro/inline func for error prints, if only to make it more obvious 
that it's an error print.

And, of course, libfmt could also be used for logging:

LOG(Camera, Error) << "Camera in " << camera_state_names[currentState]
		   << " state trying " << from << "() requiring state "
		   << camera_state_names[state];

to

LOG(Camera, Error, "Camera in {} state trying {}() requiring state {}",
     camera_state_names[currentState], from, camera_state_names[state]);

  Tomi

Naushir Patuck May 10, 2022, 9:49 a.m. UTC | #2

Hi Tomi,

On Tue, 10 May 2022 at 08:16, Tomi Valkeinen via libcamera-devel <
libcamera-devel@lists.libcamera.org> 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@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 :)

Regards,
Naush



> ---
>  src/cam/camera_session.cpp | 105 ++++++++++++++++---------------------
>  src/cam/drm.cpp            |  68 ++++++++----------------
>  src/cam/event_loop.cpp     |   9 ++--
>  src/cam/file_sink.cpp      |  31 +++++------
>  src/cam/image.cpp          |  15 +++---
>  src/cam/kms_sink.cpp       |  38 ++++++--------
>  src/cam/main.cpp           |  28 +++++-----
>  src/cam/meson.build        |   5 +-
>  src/cam/options.cpp        |  66 ++++++++++-------------
>  src/cam/stream_options.cpp |  18 +++----
>  10 files changed, 165 insertions(+), 218 deletions(-)
>
> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> index efffafbf..7843c3fd 100644
> --- a/src/cam/camera_session.cpp
> +++ b/src/cam/camera_session.cpp
> @@ -5,10 +5,9 @@
>   * camera_session.cpp - Camera capture session
>   */
>
> -#include <iomanip>
> -#include <iostream>
>  #include <limits.h>
> -#include <sstream>
> +#include <fmt/format.h>
> +#include <fmt/ostream.h>
>
>  #include <libcamera/control_ids.h>
>  #include <libcamera/property_ids.h>
> @@ -22,6 +21,9 @@
>  #include "main.h"
>  #include "stream_options.h"
>
> +#define PR(...) fmt::print(__VA_ARGS__)
> +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> +
>  using namespace libcamera;
>
>  CameraSession::CameraSession(CameraManager *cm,
> @@ -40,13 +42,12 @@ CameraSession::CameraSession(CameraManager *cm,
>                 camera_ = cm->get(cameraId);
>
>         if (!camera_) {
> -               std::cerr << "Camera " << cameraId << " not found" <<
> std::endl;
> +               EPR("Camera {} not found\n", cameraId);
>                 return;
>         }
>
>         if (camera_->acquire()) {
> -               std::cerr << "Failed to acquire camera " << cameraId
> -                         << std::endl;
> +               EPR("Failed to acquire camera {}", cameraId);
>                 return;
>         }
>
> @@ -55,15 +56,14 @@ CameraSession::CameraSession(CameraManager *cm,
>         std::unique_ptr<CameraConfiguration> config =
>                 camera_->generateConfiguration(roles);
>         if (!config || config->size() != roles.size()) {
> -               std::cerr << "Failed to get default stream configuration"
> -                         << std::endl;
> +               EPR("Failed to get default stream configuration\n");
>                 return;
>         }
>
>         /* Apply configuration if explicitly requested. */
>         if (StreamKeyValueParser::updateConfiguration(config.get(),
>
> options_[OptStream])) {
> -               std::cerr << "Failed to update configuration" << std::endl;
> +               EPR("Failed to update configuration\n");
>                 return;
>         }
>
> @@ -72,20 +72,17 @@ CameraSession::CameraSession(CameraManager *cm,
>  #ifdef HAVE_KMS
>         if (options_.isSet(OptDisplay)) {
>                 if (options_.isSet(OptFile)) {
> -                       std::cerr << "--display and --file options are
> mutually exclusive"
> -                                 << std::endl;
> +                       EPR("--display and --file options are mutually
> exclusive\n");
>                         return;
>                 }
>
>                 if (roles.size() != 1) {
> -                       std::cerr << "Display doesn't support multiple
> streams"
> -                                 << std::endl;
> +                       EPR("Display doesn't support multiple streams\n");
>                         return;
>                 }
>
>                 if (roles[0] != StreamRole::Viewfinder) {
> -                       std::cerr << "Display requires a viewfinder stream"
> -                                 << std::endl;
> +                       EPR("Display requires a viewfinder stream\n");
>                         return;
>                 }
>         }
> @@ -97,15 +94,14 @@ CameraSession::CameraSession(CameraManager *cm,
>
>         case CameraConfiguration::Adjusted:
>                 if (strictFormats) {
> -                       std::cout << "Adjusting camera configuration
> disallowed by --strict-formats argument"
> -                                 << std::endl;
> +                       PR("Adjusting camera configuration disallowed by
> --strict-formats argument\n");
>                         return;
>                 }
> -               std::cout << "Camera configuration adjusted" << std::endl;
> +               PR("Camera configuration adjusted\n");
>                 break;
>
>         case CameraConfiguration::Invalid:
> -               std::cout << "Camera configuration invalid" << std::endl;
> +               PR("Camera configuration invalid\n");
>                 return;
>         }
>
> @@ -121,8 +117,7 @@ CameraSession::~CameraSession()
>  void CameraSession::listControls() const
>  {
>         for (const auto &[id, info] : camera_->controls()) {
> -               std::cout << "Control: " << id->name() << ": "
> -                         << info.toString() << std::endl;
> +               PR("Control: {}: {}}n", id->name(), info.toString());
>         }
>  }
>
> @@ -131,8 +126,7 @@ void CameraSession::listProperties() const
>         for (const auto &[key, value] : camera_->properties()) {
>                 const ControlId *id = properties::properties.at(key);
>
> -               std::cout << "Property: " << id->name() << " = "
> -                         << value.toString() << std::endl;
> +               PR("Property: {} = {}\n", id->name(), value.toString());
>         }
>  }
>
> @@ -140,17 +134,15 @@ void CameraSession::infoConfiguration() const
>  {
>         unsigned int index = 0;
>         for (const StreamConfiguration &cfg : *config_) {
> -               std::cout << index << ": " << cfg.toString() << std::endl;
> +               PR("{}: {}\n", index, cfg.toString());
>
>                 const StreamFormats &formats = cfg.formats();
>                 for (PixelFormat pixelformat : formats.pixelformats()) {
> -                       std::cout << " * Pixelformat: "
> -                                 << pixelformat << " "
> -                                 << formats.range(pixelformat).toString()
> -                                 << std::endl;
> +                       PR(" * Pixelformat: {} {}\n", pixelformat,
> +                          formats.range(pixelformat).toString());
>
>                         for (const Size &size : formats.sizes(pixelformat))
> -                               std::cout << "  - " << size << std::endl;
> +                               PR("  - {}\n", size);
>                 }
>
>                 index++;
> @@ -168,7 +160,7 @@ int CameraSession::start()
>
>         ret = camera_->configure(config_.get());
>         if (ret < 0) {
> -               std::cout << "Failed to configure camera" << std::endl;
> +               PR("Failed to configure camera\n");
>                 return ret;
>         }
>
> @@ -197,8 +189,7 @@ int CameraSession::start()
>         if (sink_) {
>                 ret = sink_->configure(*config_);
>                 if (ret < 0) {
> -                       std::cout << "Failed to configure frame sink"
> -                                 << std::endl;
> +                       PR("Failed to configure frame sink\n");
>                         return ret;
>                 }
>
> @@ -214,12 +205,12 @@ void CameraSession::stop()
>  {
>         int ret = camera_->stop();
>         if (ret)
> -               std::cout << "Failed to stop capture" << std::endl;
> +               PR("Failed to stop capture\n");
>
>         if (sink_) {
>                 ret = sink_->stop();
>                 if (ret)
> -                       std::cout << "Failed to stop frame sink" <<
> std::endl;
> +                       PR("Failed to stop frame sink\n");
>         }
>
>         sink_.reset();
> @@ -238,7 +229,7 @@ int CameraSession::startCapture()
>         for (StreamConfiguration &cfg : *config_) {
>                 ret = allocator_->allocate(cfg.stream());
>                 if (ret < 0) {
> -                       std::cerr << "Can't allocate buffers" << std::endl;
> +                       EPR("Can't allocate buffers\n");
>                         return -ENOMEM;
>                 }
>
> @@ -254,7 +245,7 @@ int CameraSession::startCapture()
>         for (unsigned int i = 0; i < nbuffers; i++) {
>                 std::unique_ptr<Request> request =
> camera_->createRequest();
>                 if (!request) {
> -                       std::cerr << "Can't create request" << std::endl;
> +                       EPR("Can't create request\n");
>                         return -ENOMEM;
>                 }
>
> @@ -266,8 +257,7 @@ int CameraSession::startCapture()
>
>                         ret = request->addBuffer(stream, buffer.get());
>                         if (ret < 0) {
> -                               std::cerr << "Can't set buffer for request"
> -                                         << std::endl;
> +                               EPR("Can't set buffer for request\n");
>                                 return ret;
>                         }
>
> @@ -281,14 +271,14 @@ int CameraSession::startCapture()
>         if (sink_) {
>                 ret = sink_->start();
>                 if (ret) {
> -                       std::cout << "Failed to start frame sink" <<
> std::endl;
> +                       PR("Failed to start frame sink\n");
>                         return ret;
>                 }
>         }
>
>         ret = camera_->start();
>         if (ret) {
> -               std::cout << "Failed to start capture" << std::endl;
> +               PR("Failed to start capture\n");
>                 if (sink_)
>                         sink_->stop();
>                 return ret;
> @@ -297,7 +287,7 @@ int CameraSession::startCapture()
>         for (std::unique_ptr<Request> &request : requests_) {
>                 ret = queueRequest(request.get());
>                 if (ret < 0) {
> -                       std::cerr << "Can't queue request" << std::endl;
> +                       EPR("Can't queue request\n");
>                         camera_->stop();
>                         if (sink_)
>                                 sink_->stop();
> @@ -306,13 +296,11 @@ int CameraSession::startCapture()
>         }
>
>         if (captureLimit_)
> -               std::cout << "cam" << cameraIndex_
> -                         << ": Capture " << captureLimit_ << " frames"
> -                         << std::endl;
> +               PR("cam{}: Capture {} frames\n", cameraIndex_,
> +                  captureLimit_);
>         else
> -               std::cout << "cam" << cameraIndex_
> -                         << ": Capture until user interrupts by SIGINT"
> -                         << std::endl;
> +               PR("cam{}: Capture until user interrupts by SIGINT\n",
> +                  cameraIndex_);
>
>         return 0;
>  }
> @@ -364,23 +352,23 @@ void CameraSession::processRequest(Request *request)
>
>         bool requeue = true;
>
> -       std::stringstream info;
> -       info << ts / 1000000000 << "."
> -            << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
> -            << " (" << std::fixed << std::setprecision(2) << fps << "
> fps)";
> +       auto sbuf = fmt::memory_buffer();
> +       fmt::format_to(std::back_inserter(sbuf), "{}.{:06} ({:.2f} fps)",
> +                      ts / 1000000000,
> +                      ts / 1000 % 1000000,
> +                      fps);
>
>         for (const auto &[stream, buffer] : buffers) {
>                 const FrameMetadata &metadata = buffer->metadata();
>
> -               info << " " << streamNames_[stream]
> -                    << " seq: " << std::setw(6) << std::setfill('0') <<
> metadata.sequence
> -                    << " bytesused: ";
> +               fmt::format_to(std::back_inserter(sbuf), " {} seq: {:06}
> bytesused: ",
> +                              streamNames_[stream], metadata.sequence);
>
>                 unsigned int nplane = 0;
>                 for (const FrameMetadata::Plane &plane :
> metadata.planes()) {
> -                       info << plane.bytesused;
> +                       fmt::format_to(std::back_inserter(sbuf), "{}",
> plane.bytesused);
>                         if (++nplane < metadata.planes().size())
> -                               info << "/";
> +                               fmt::format_to(std::back_inserter(sbuf),
> "/");
>                 }
>         }
>
> @@ -389,14 +377,13 @@ void CameraSession::processRequest(Request *request)
>                         requeue = false;
>         }
>
> -       std::cout << info.str() << std::endl;
> +       PR("{}\n", fmt::to_string(sbuf));
>
>         if (printMetadata_) {
>                 const ControlList &requestMetadata = request->metadata();
>                 for (const auto &[key, value] : requestMetadata) {
>                         const ControlId *id = controls::controls.at(key);
> -                       std::cout << "\t" << id->name() << " = "
> -                                 << value.toString() << std::endl;
> +                       PR("\t{} = {}\n", id->name(), value.toString());
>                 }
>         }
>
> diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> index 46e34eb5..84919ab3 100644
> --- a/src/cam/drm.cpp
> +++ b/src/cam/drm.cpp
> @@ -10,12 +10,12 @@
>  #include <algorithm>
>  #include <errno.h>
>  #include <fcntl.h>
> -#include <iostream>
>  #include <set>
>  #include <string.h>
>  #include <sys/ioctl.h>
>  #include <sys/stat.h>
>  #include <sys/types.h>
> +#include <fmt/core.h>
>
>  #include <libcamera/framebuffer.h>
>  #include <libcamera/geometry.h>
> @@ -25,6 +25,9 @@
>
>  #include "event_loop.h"
>
> +#define PR(...) fmt::print(__VA_ARGS__)
> +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> +
>  namespace DRM {
>
>  Object::Object(Device *dev, uint32_t id, Type type)
> @@ -178,9 +181,7 @@ Connector::Connector(Device *dev, const
> drmModeConnector *connector)
>  {
>         auto typeName = connectorTypeNames.find(connector->connector_type);
>         if (typeName == connectorTypeNames.end()) {
> -               std::cerr
> -                       << "Invalid connector type "
> -                       << connector->connector_type << std::endl;
> +               EPR("Invalid connector type {}}n",
> connector->connector_type);
>                 typeName =
> connectorTypeNames.find(DRM_MODE_CONNECTOR_Unknown);
>         }
>
> @@ -213,9 +214,7 @@ Connector::Connector(Device *dev, const
> drmModeConnector *connector)
>                                                     return e.id() ==
> encoderId;
>                                             });
>                 if (encoder == encoders.end()) {
> -                       std::cerr
> -                               << "Encoder " << encoderId << " not found"
> -                               << std::endl;
> +                       EPR("Encoder {} not found\n", encoderId);
>                         continue;
>                 }
>
> @@ -296,9 +295,7 @@ FrameBuffer::~FrameBuffer()
>
>                 if (ret == -1) {
>                         ret = -errno;
> -                       std::cerr
> -                               << "Failed to close GEM object: "
> -                               << strerror(-ret) << std::endl;
> +                       EPR("Failed to close GEM object: {}\n",
> strerror(-ret));
>                 }
>         }
>
> @@ -408,9 +405,8 @@ int Device::init()
>         fd_ = open(name, O_RDWR | O_CLOEXEC);
>         if (fd_ < 0) {
>                 ret = -errno;
> -               std::cerr
> -                       << "Failed to open DRM/KMS device " << name << ": "
> -                       << strerror(-ret) << std::endl;
> +               EPR("Failed to open DRM/KMS device {}: {}\n", name,
> +                   strerror(-ret));
>                 return ret;
>         }
>
> @@ -421,9 +417,7 @@ int Device::init()
>         ret = drmSetClientCap(fd_, DRM_CLIENT_CAP_ATOMIC, 1);
>         if (ret < 0) {
>                 ret = -errno;
> -               std::cerr
> -                       << "Failed to enable atomic capability: "
> -                       << strerror(-ret) << std::endl;
> +               EPR("Failed to enable atomic capability: {}\n",
> strerror(-ret));
>                 return ret;
>         }
>
> @@ -448,9 +442,7 @@ int Device::getResources()
>         };
>         if (!resources) {
>                 ret = -errno;
> -               std::cerr
> -                       << "Failed to get DRM/KMS resources: "
> -                       << strerror(-ret) << std::endl;
> +               EPR("Failed to get DRM/KMS resources: {}\n",
> strerror(-ret));
>                 return ret;
>         }
>
> @@ -458,9 +450,7 @@ int Device::getResources()
>                 drmModeCrtc *crtc = drmModeGetCrtc(fd_,
> resources->crtcs[i]);
>                 if (!crtc) {
>                         ret = -errno;
> -                       std::cerr
> -                               << "Failed to get CRTC: " << strerror(-ret)
> -                               << std::endl;
> +                       EPR("Failed to get CRTC: {}\n", strerror(-ret));
>                         return ret;
>                 }
>
> @@ -476,9 +466,7 @@ int Device::getResources()
>                         drmModeGetEncoder(fd_, resources->encoders[i]);
>                 if (!encoder) {
>                         ret = -errno;
> -                       std::cerr
> -                               << "Failed to get encoder: " <<
> strerror(-ret)
> -                               << std::endl;
> +                       EPR("Failed to get encoder: {}\n", strerror(-ret));
>                         return ret;
>                 }
>
> @@ -494,9 +482,7 @@ int Device::getResources()
>                         drmModeGetConnector(fd_, resources->connectors[i]);
>                 if (!connector) {
>                         ret = -errno;
> -                       std::cerr
> -                               << "Failed to get connector: " <<
> strerror(-ret)
> -                               << std::endl;
> +                       EPR("Failed to get connector: {}\n",
> strerror(-ret));
>                         return ret;
>                 }
>
> @@ -513,9 +499,7 @@ int Device::getResources()
>         };
>         if (!planes) {
>                 ret = -errno;
> -               std::cerr
> -                       << "Failed to get DRM/KMS planes: "
> -                       << strerror(-ret) << std::endl;
> +               EPR("Failed to get DRM/KMS planes: {}\n", strerror(-ret));
>                 return ret;
>         }
>
> @@ -524,9 +508,7 @@ int Device::getResources()
>                         drmModeGetPlane(fd_, planes->planes[i]);
>                 if (!plane) {
>                         ret = -errno;
> -                       std::cerr
> -                               << "Failed to get plane: " <<
> strerror(-ret)
> -                               << std::endl;
> +                       EPR("Failed to get plane: {}\n", strerror(-ret));
>                         return ret;
>                 }
>
> @@ -556,9 +538,7 @@ int Device::getResources()
>                 drmModePropertyRes *property = drmModeGetProperty(fd_, id);
>                 if (!property) {
>                         ret = -errno;
> -                       std::cerr
> -                               << "Failed to get property: " <<
> strerror(-ret)
> -                               << std::endl;
> +                       EPR("Failed to get property: {}\n",
> strerror(-ret));
>                         continue;
>                 }
>
> @@ -573,9 +553,8 @@ int Device::getResources()
>         for (auto &object : objects_) {
>                 ret = object.second->setup();
>                 if (ret < 0) {
> -                       std::cerr
> -                               << "Failed to setup object " <<
> object.second->id()
> -                               << ": " << strerror(-ret) << std::endl;
> +                       EPR("Failed to setup object {}: {}\n",
> +                           object.second->id(), strerror(-ret));
>                         return ret;
>                 }
>         }
> @@ -616,9 +595,8 @@ std::unique_ptr<FrameBuffer> Device::createFrameBuffer(
>                         ret = drmPrimeFDToHandle(fd_, plane.fd.get(),
> &handle);
>                         if (ret < 0) {
>                                 ret = -errno;
> -                               std::cerr
> -                                       << "Unable to import framebuffer
> dmabuf: "
> -                                       << strerror(-ret) << std::endl;
> +                               EPR("Unable to import framebuffer dmabuf:
> {}\n",
> +                                   strerror(-ret));
>                                 return nullptr;
>                         }
>
> @@ -636,9 +614,7 @@ std::unique_ptr<FrameBuffer> Device::createFrameBuffer(
>                             strides.data(), offsets, &fb->id_, 0);
>         if (ret < 0) {
>                 ret = -errno;
> -               std::cerr
> -                       << "Failed to add framebuffer: "
> -                       << strerror(-ret) << std::endl;
> +               EPR("Failed to add framebuffer: {}\n", strerror(-ret));
>                 return nullptr;
>         }
>
> diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> index e25784c0..87aaf59a 100644
> --- a/src/cam/event_loop.cpp
> +++ b/src/cam/event_loop.cpp
> @@ -10,7 +10,10 @@
>  #include <assert.h>
>  #include <event2/event.h>
>  #include <event2/thread.h>
> -#include <iostream>
> +#include <fmt/core.h>
> +
> +#define PR(...) fmt::print(__VA_ARGS__)
> +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
>
>  EventLoop *EventLoop::instance_ = nullptr;
>
> @@ -71,13 +74,13 @@ void EventLoop::addEvent(int fd, EventType type,
>         event->event_ = event_new(base_, fd, events,
> &EventLoop::Event::dispatch,
>                                   event.get());
>         if (!event->event_) {
> -               std::cerr << "Failed to create event for fd " << fd <<
> std::endl;
> +               EPR("Failed to create event for fd {}\n", fd);
>                 return;
>         }
>
>         int ret = event_add(event->event_, nullptr);
>         if (ret < 0) {
> -               std::cerr << "Failed to add event for fd " << fd <<
> std::endl;
> +               EPR("Failed to add event for fd {}\n", fd);
>                 return;
>         }
>
> diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp
> index 45213d4a..86e2118c 100644
> --- a/src/cam/file_sink.cpp
> +++ b/src/cam/file_sink.cpp
> @@ -7,11 +7,12 @@
>
>  #include <assert.h>
>  #include <fcntl.h>
> -#include <iomanip>
> -#include <iostream>
> -#include <sstream>
>  #include <string.h>
>  #include <unistd.h>
> +#include <fmt/core.h>
> +
> +#define PR(...) fmt::print(__VA_ARGS__)
> +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
>
>  #include <libcamera/camera.h>
>
> @@ -70,10 +71,10 @@ void FileSink::writeBuffer(const Stream *stream,
> FrameBuffer *buffer)
>
>         pos = filename.find_first_of('#');
>         if (pos != std::string::npos) {
> -               std::stringstream ss;
> -               ss << streamNames_[stream] << "-" << std::setw(6)
> -                  << std::setfill('0') << buffer->metadata().sequence;
> -               filename.replace(pos, 1, ss.str());
> +               auto s = fmt::format("{}-{:06}",
> +                                    streamNames_[stream],
> +                                    buffer->metadata().sequence);
> +               filename.replace(pos, 1, s);
>         }
>
>         fd = open(filename.c_str(), O_CREAT | O_WRONLY |
> @@ -81,8 +82,7 @@ void FileSink::writeBuffer(const Stream *stream,
> FrameBuffer *buffer)
>                   S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH |
> S_IWOTH);
>         if (fd == -1) {
>                 ret = -errno;
> -               std::cerr << "failed to open file " << filename << ": "
> -                         << strerror(-ret) << std::endl;
> +               EPR("failed to open file {}: {}\n", filename,
> strerror(-ret));
>                 return;
>         }
>
> @@ -95,20 +95,17 @@ void FileSink::writeBuffer(const Stream *stream,
> FrameBuffer *buffer)
>                 unsigned int length = std::min<unsigned
> int>(meta.bytesused, data.size());
>
>                 if (meta.bytesused > data.size())
> -                       std::cerr << "payload size " << meta.bytesused
> -                                 << " larger than plane size " <<
> data.size()
> -                                 << std::endl;
> +                       EPR("payload size {} larger than plane size {}\n",
> +                           meta.bytesused, data.size());
>
>                 ret = ::write(fd, data.data(), length);
>                 if (ret < 0) {
>                         ret = -errno;
> -                       std::cerr << "write error: " << strerror(-ret)
> -                                 << std::endl;
> +                       EPR("write error: {}\n", strerror(-ret));
>                         break;
>                 } else if (ret != (int)length) {
> -                       std::cerr << "write error: only " << ret
> -                                 << " bytes written instead of "
> -                                 << length << std::endl;
> +                       EPR("write error: only {} bytes written instead of
> {}\n",
> +                           ret, length);
>                         break;
>                 }
>         }
> diff --git a/src/cam/image.cpp b/src/cam/image.cpp
> index fe2cc6da..73bcf915 100644
> --- a/src/cam/image.cpp
> +++ b/src/cam/image.cpp
> @@ -9,11 +9,14 @@
>
>  #include <assert.h>
>  #include <errno.h>
> -#include <iostream>
>  #include <map>
>  #include <string.h>
>  #include <sys/mman.h>
>  #include <unistd.h>
> +#include <fmt/core.h>
> +
> +#define PR(...) fmt::print(__VA_ARGS__)
> +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
>
>  using namespace libcamera;
>
> @@ -49,10 +52,8 @@ std::unique_ptr<Image> Image::fromFrameBuffer(const
> FrameBuffer *buffer, MapMode
>
>                 if (plane.offset > length ||
>                     plane.offset + plane.length > length) {
> -                       std::cerr << "plane is out of buffer: buffer
> length="
> -                                 << length << ", plane offset=" <<
> plane.offset
> -                                 << ", plane length=" << plane.length
> -                                 << std::endl;
> +                       EPR("plane is out of buffer: buffer length={},
> plane offset={}, plane length={}\n",
> +                           length, plane.offset, plane.length);
>                         return nullptr;
>                 }
>                 size_t &mapLength = mappedBuffers[fd].mapLength;
> @@ -68,8 +69,8 @@ std::unique_ptr<Image> Image::fromFrameBuffer(const
> FrameBuffer *buffer, MapMode
>                                              MAP_SHARED, fd, 0);
>                         if (address == MAP_FAILED) {
>                                 int error = -errno;
> -                               std::cerr << "Failed to mmap plane: "
> -                                         << strerror(-error) << std::endl;
> +                               EPR("Failed to mmap plane: {}\n",
> +                                   strerror(-error));
>                                 return nullptr;
>                         }
>
> diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> index 7add81a6..823b75e4 100644
> --- a/src/cam/kms_sink.cpp
> +++ b/src/cam/kms_sink.cpp
> @@ -10,10 +10,13 @@
>  #include <array>
>  #include <algorithm>
>  #include <assert.h>
> -#include <iostream>
>  #include <memory>
>  #include <stdint.h>
>  #include <string.h>
> +#include <fmt/core.h>
> +
> +#define PR(...) fmt::print(__VA_ARGS__)
> +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
>
>  #include <libcamera/camera.h>
>  #include <libcamera/formats.h>
> @@ -54,11 +57,9 @@ KMSSink::KMSSink(const std::string &connectorName)
>
>         if (!connector_) {
>                 if (!connectorName.empty())
> -                       std::cerr
> -                               << "Connector " << connectorName << " not
> found"
> -                               << std::endl;
> +                       EPR("Connector {} not found\n", connectorName);
>                 else
> -                       std::cerr << "No connected connector found" <<
> std::endl;
> +                       EPR("No connected connector found\n");
>                 return;
>         }
>
> @@ -119,7 +120,7 @@ int KMSSink::configure(const
> libcamera::CameraConfiguration &config)
>                                                       mode.vdisplay ==
> cfg.size.height;
>                                        });
>         if (iter == modes.end()) {
> -               std::cerr << "No mode matching " << cfg.size << std::endl;
> +               EPR("No mode matching {}\n", cfg.size);
>                 return -EINVAL;
>         }
>
> @@ -192,17 +193,12 @@ int KMSSink::configurePipeline(const
> libcamera::PixelFormat &format)
>  {
>         const int ret = selectPipeline(format);
>         if (ret) {
> -               std::cerr
> -                       << "Unable to find display pipeline for format "
> -                       << format << std::endl;
> -
> +               EPR("Unable to find display pipeline for format {}\n",
> format);
>                 return ret;
>         }
>
> -       std::cout
> -               << "Using KMS plane " << plane_->id() << ", CRTC " <<
> crtc_->id()
> -               << ", connector " << connector_->name()
> -               << " (" << connector_->id() << ")" << std::endl;
> +       PR("Using KMS plane {}, CRTC {}, connector {} ({})\n",
> +          plane_->id(), crtc_->id(), connector_->name(),
> connector_->id());
>
>         return 0;
>  }
> @@ -228,9 +224,8 @@ int KMSSink::start()
>
>         ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
>         if (ret < 0) {
> -               std::cerr
> -                       << "Failed to disable CRTCs and planes: "
> -                       << strerror(-ret) << std::endl;
> +               EPR("Failed to disable CRTCs and planes: {}\n",
> +                   strerror(-ret));
>                 return ret;
>         }
>
> @@ -250,9 +245,7 @@ int KMSSink::stop()
>
>         int ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);
>         if (ret < 0) {
> -               std::cerr
> -                       << "Failed to stop display pipeline: "
> -                       << strerror(-ret) << std::endl;
> +               EPR("Failed to stop display pipeline: {}\n",
> strerror(-ret));
>                 return ret;
>         }
>
> @@ -312,9 +305,8 @@ bool KMSSink::processRequest(libcamera::Request
> *camRequest)
>         if (!queued_) {
>                 int ret = drmRequest->commit(flags);
>                 if (ret < 0) {
> -                       std::cerr
> -                               << "Failed to commit atomic request: "
> -                               << strerror(-ret) << std::endl;
> +                       EPR("Failed to commit atomic request: {}\n",
> +                           strerror(-ret));
>                         /* \todo Implement error handling */
>                 }
>
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index c7f664b9..03615dc9 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -6,10 +6,9 @@
>   */
>
>  #include <atomic>
> -#include <iomanip>
> -#include <iostream>
>  #include <signal.h>
>  #include <string.h>
> +#include <fmt/core.h>
>
>  #include <libcamera/libcamera.h>
>  #include <libcamera/property_ids.h>
> @@ -78,8 +77,7 @@ int CamApp::init(int argc, char **argv)
>
>         ret = cm_->start();
>         if (ret) {
> -               std::cout << "Failed to start camera manager: "
> -                         << strerror(-ret) << std::endl;
> +               fmt::print("Failed to start camera manager: {}\n", -ret);
>                 return ret;
>         }
>
> @@ -173,12 +171,12 @@ int CamApp::parseOptions(int argc, char *argv[])
>
>  void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
>  {
> -       std::cout << "Camera Added: " << cam->id() << std::endl;
> +       fmt::print("Camera Added: {}\n", cam->id());
>  }
>
>  void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
>  {
> -       std::cout << "Camera Removed: " << cam->id() << std::endl;
> +       fmt::print("Camera Removed: {}\n", cam->id());
>  }
>
>  void CamApp::captureDone()
> @@ -193,11 +191,11 @@ int CamApp::run()
>
>         /* 1. List all cameras. */
>         if (options_.isSet(OptList)) {
> -               std::cout << "Available cameras:" << std::endl;
> +               fmt::print("Available cameras:\n");
>
>                 unsigned int index = 1;
>                 for (const std::shared_ptr<Camera> &cam : cm_->cameras()) {
> -                       std::cout << index << ": " <<
> cameraName(cam.get()) << std::endl;
> +                       fmt::print("{}: {}\n", cameraName(cam.get()),
> index);
>                         index++;
>                 }
>         }
> @@ -215,12 +213,12 @@ int CamApp::run()
>                                                                 index,
>
> camera.children());
>                         if (!session->isValid()) {
> -                               std::cout << "Failed to create camera
> session" << std::endl;
> +                               fmt::print("Failed to create camera
> session\n");
>                                 return -EINVAL;
>                         }
>
> -                       std::cout << "Using camera " <<
> session->camera()->id()
> -                                 << " as cam" << index << std::endl;
> +                       fmt::print("Using camera{} as cam{}\n",
> +                                  session->camera()->id(), index);
>
>                         session->captureDone.connect(this,
> &CamApp::captureDone);
>
> @@ -250,7 +248,7 @@ int CamApp::run()
>
>                 ret = session->start();
>                 if (ret) {
> -                       std::cout << "Failed to start camera session" <<
> std::endl;
> +                       fmt::print("Failed to start camera session\n");
>                         return ret;
>                 }
>
> @@ -259,8 +257,8 @@ int CamApp::run()
>
>         /* 5. Enable hotplug monitoring. */
>         if (options_.isSet(OptMonitor)) {
> -               std::cout << "Monitoring new hotplug and unplug events" <<
> std::endl;
> -               std::cout << "Press Ctrl-C to interrupt" << std::endl;
> +               fmt::print("Monitoring new hotplug and unplug events\n");
> +               fmt::print("Press Ctrl-C to interrupt\n");
>
>                 cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
>                 cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> @@ -323,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)
>
>  void signalHandler([[maybe_unused]] int signal)
>  {
> -       std::cout << "Exiting" << std::endl;
> +       fmt::print("Exiting");
>         CamApp::instance()->quit();
>  }
>
> diff --git a/src/cam/meson.build b/src/cam/meson.build
> index 5bab8c9e..2b47383d 100644
> --- a/src/cam/meson.build
> +++ b/src/cam/meson.build
> @@ -7,6 +7,8 @@ if not libevent.found()
>      subdir_done()
>  endif
>
> +libfmt_dep = dependency('fmt')
> +
>  cam_enabled = true
>
>  cam_sources = files([
> @@ -25,7 +27,7 @@ cam_cpp_args = []
>  libdrm = dependency('libdrm', required : false)
>
>  if libdrm.found()
> -    cam_cpp_args += [ '-DHAVE_KMS' ]
> +    cam_cpp_args += [ '-DHAVE_KMS', ]
>      cam_sources += files([
>          'drm.cpp',
>          'kms_sink.cpp'
> @@ -38,6 +40,7 @@ cam  = executable('cam', cam_sources,
>                        libcamera_public,
>                        libdrm,
>                        libevent,
> +                      libfmt_dep,
>                    ],
>                    cpp_args : cam_cpp_args,
>                    install : true)
> diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> index 4f7e8691..c9979385 100644
> --- a/src/cam/options.cpp
> +++ b/src/cam/options.cpp
> @@ -7,9 +7,11 @@
>
>  #include <assert.h>
>  #include <getopt.h>
> -#include <iomanip>
> -#include <iostream>
>  #include <string.h>
> +#include <fmt/core.h>
> +
> +#define PR(...) fmt::print(__VA_ARGS__)
> +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
>
>  #include "options.h"
>
> @@ -390,26 +392,23 @@ KeyValueParser::Options KeyValueParser::parse(const
> char *arguments)
>                         continue;
>
>                 if (optionsMap_.find(key) == optionsMap_.end()) {
> -                       std::cerr << "Invalid option " << key << std::endl;
> +                       EPR("Invalid option {}\n", key);
>                         return options;
>                 }
>
>                 OptionArgument arg = optionsMap_[key].argument;
>                 if (value.empty() && arg == ArgumentRequired) {
> -                       std::cerr << "Option " << key << " requires an
> argument"
> -                                 << std::endl;
> +                       EPR("Option {} requires an argument\n", key);
>                         return options;
>                 } else if (!value.empty() && arg == ArgumentNone) {
> -                       std::cerr << "Option " << key << " takes no
> argument"
> -                                 << std::endl;
> +                       EPR("Option {} takes no argument\n", key);
>                         return options;
>                 }
>
>                 const Option &option = optionsMap_[key];
>                 if (!options.parseValue(key, option, value.c_str())) {
> -                       std::cerr << "Failed to parse '" << value << "' as
> "
> -                                 << option.typeName() << " for option "
> << key
> -                                 << std::endl;
> +                       EPR("Failed to parse '{}' as {} for option {}\n",
> +                           value, option.typeName(), key);
>                         return options;
>                 }
>         }
> @@ -453,16 +452,16 @@ void KeyValueParser::usage(int indent)
>                                 argument += "]";
>                 }
>
> -               std::cerr << std::setw(indent) << argument;
> +               EPR("{:{}}", argument, indent);
>
>                 for (const char *help = option.help, *end = help; end;) {
>                         end = strchr(help, '\n');
>                         if (end) {
> -                               std::cerr << std::string(help, end - help
> + 1);
> -                               std::cerr << std::setw(indent) << " ";
> +                               EPR(std::string(help, end - help + 1));
> +                               EPR("{:{}}", "", indent);
>                                 help = end + 1;
>                         } else {
> -                               std::cerr << help << std::endl;
> +                               EPR("{}\n", help);
>                         }
>                 }
>         }
> @@ -929,10 +928,10 @@ OptionsParser::Options OptionsParser::parse(int
> argc, char **argv)
>
>                 if (c == '?' || c == ':') {
>                         if (c == '?')
> -                               std::cerr << "Invalid option ";
> +                               EPR("Invalid option ");
>                         else
> -                               std::cerr << "Missing argument for option
> ";
> -                       std::cerr << argv[optind - 1] << std::endl;
> +                               EPR("Missing argument for option ");
> +                       EPR("{}\n", argv[optind - 1]);
>
>                         usage();
>                         return options;
> @@ -946,8 +945,7 @@ OptionsParser::Options OptionsParser::parse(int argc,
> char **argv)
>         }
>
>         if (optind < argc) {
> -               std::cerr << "Invalid non-option argument '" <<
> argv[optind]
> -                         << "'" << std::endl;
> +               EPR("Invalid non-option argument '{}'\n", argv[optind]);
>                 usage();
>                 return options;
>         }
> @@ -992,14 +990,9 @@ void OptionsParser::usage()
>
>         indent = (indent + 7) / 8 * 8;
>
> -       std::cerr << "Options:" << std::endl;
> -
> -       std::ios_base::fmtflags f(std::cerr.flags());
> -       std::cerr << std::left;
> +       EPR("Options:\n");
>
>         usageOptions(options_, indent);
> -
> -       std::cerr.flags(f);
>  }
>
>  void OptionsParser::usageOptions(const std::list<Option> &options,
> @@ -1036,16 +1029,16 @@ void OptionsParser::usageOptions(const
> std::list<Option> &options,
>                 if (option.isArray)
>                         argument += " ...";
>
> -               std::cerr << std::setw(indent) << argument;
> +               EPR("{:{}}", argument, indent);
>
> -               for (const char *help = option.help, *end = help; end; ) {
> +               for (const char *help = option.help, *end = help; end;) {
>                         end = strchr(help, '\n');
>                         if (end) {
> -                               std::cerr << std::string(help, end - help
> + 1);
> -                               std::cerr << std::setw(indent) << " ";
> +                               EPR(std::string(help, end - help + 1));
> +                               EPR("{:{}}", "", indent);
>                                 help = end + 1;
>                         } else {
> -                               std::cerr << help << std::endl;
> +                               EPR("{}\n", help);
>                         }
>                 }
>
> @@ -1060,8 +1053,8 @@ void OptionsParser::usageOptions(const
> std::list<Option> &options,
>                 return;
>
>         for (const Option *option : parentOptions) {
> -               std::cerr << std::endl << "Options valid in the context of
> "
> -                         << option->optionName() << ":" << std::endl;
> +               EPR("\nOptions valid in the context of {}:\n",
> +                   option->optionName());
>                 usageOptions(option->children, indent);
>         }
>  }
> @@ -1125,15 +1118,14 @@ bool OptionsParser::parseValue(const Option
> &option, const char *arg,
>
>         std::tie(options, error) = childOption(option.parent, options);
>         if (error) {
> -               std::cerr << "Option " << option.optionName() << "
> requires a "
> -                         << error->optionName() << " context" <<
> std::endl;
> +               EPR("Option {} requires a {} context\n",
> +                   option.optionName(), error->optionName());
>                 return false;
>         }
>
>         if (!options->parseValue(option.opt, option, arg)) {
> -               std::cerr << "Can't parse " << option.typeName()
> -                         << " argument for option " << option.optionName()
> -                         << std::endl;
> +               EPR("Can't parse {} argument for option {}\n",
> +                   option.typeName(), option.optionName());
>                 return false;
>         }
>
> diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp
> index 150bd27c..666862eb 100644
> --- a/src/cam/stream_options.cpp
> +++ b/src/cam/stream_options.cpp
> @@ -6,7 +6,10 @@
>   */
>  #include "stream_options.h"
>
> -#include <iostream>
> +#include <fmt/core.h>
> +
> +#define PR(...) fmt::print(__VA_ARGS__)
> +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
>
>  using namespace libcamera;
>
> @@ -30,8 +33,7 @@ KeyValueParser::Options
> StreamKeyValueParser::parse(const char *arguments)
>
>         if (options.valid() && options.isSet("role") &&
>             !parseRole(&role, options)) {
> -               std::cerr << "Unknown stream role "
> -                         << options["role"].toString() << std::endl;
> +               EPR("Unknown stream role {}\n",
> options["role"].toString());
>                 options.invalidate();
>         }
>
> @@ -64,7 +66,7 @@ int
> StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
>                                               const OptionValue &values)
>  {
>         if (!config) {
> -               std::cerr << "No configuration provided" << std::endl;
> +               EPR("No configuration provided\n");
>                 return -EINVAL;
>         }
>
> @@ -75,12 +77,8 @@ int
> StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
>         const std::vector<OptionValue> &streamParameters =
> values.toArray();
>
>         if (config->size() != streamParameters.size()) {
> -               std::cerr
> -                       << "Number of streams in configuration "
> -                       << config->size()
> -                       << " does not match number of streams parsed "
> -                       << streamParameters.size()
> -                       << std::endl;
> +               EPR("Number of streams in configuration {} does not match
> number of streams parsed {}\n",
> +                   config->size(), streamParameters.size());
>                 return -EINVAL;
>         }
>
> --
> 2.34.1
>
>

Kieran Bingham May 10, 2022, 10:09 a.m. UTC | #3

Quoting Naushir Patuck (2022-05-10 10:49:14)
> Hi Tomi,
> 
> On Tue, 10 May 2022 at 08:16, Tomi Valkeinen via libcamera-devel <
> libcamera-devel@lists.libcamera.org> 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@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 :)

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.

--
Kieran


> 
> Regards,
> Naush
> 
> 
> 
> > ---
> >  src/cam/camera_session.cpp | 105 ++++++++++++++++---------------------
> >  src/cam/drm.cpp            |  68 ++++++++----------------
> >  src/cam/event_loop.cpp     |   9 ++--
> >  src/cam/file_sink.cpp      |  31 +++++------
> >  src/cam/image.cpp          |  15 +++---
> >  src/cam/kms_sink.cpp       |  38 ++++++--------
> >  src/cam/main.cpp           |  28 +++++-----
> >  src/cam/meson.build        |   5 +-
> >  src/cam/options.cpp        |  66 ++++++++++-------------
> >  src/cam/stream_options.cpp |  18 +++----
> >  10 files changed, 165 insertions(+), 218 deletions(-)
> >
> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > index efffafbf..7843c3fd 100644
> > --- a/src/cam/camera_session.cpp
> > +++ b/src/cam/camera_session.cpp
> > @@ -5,10 +5,9 @@
> >   * camera_session.cpp - Camera capture session
> >   */
> >
> > -#include <iomanip>
> > -#include <iostream>
> >  #include <limits.h>
> > -#include <sstream>
> > +#include <fmt/format.h>
> > +#include <fmt/ostream.h>
> >
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/property_ids.h>
> > @@ -22,6 +21,9 @@
> >  #include "main.h"
> >  #include "stream_options.h"
> >
> > +#define PR(...) fmt::print(__VA_ARGS__)
> > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> > +
> >  using namespace libcamera;
> >
> >  CameraSession::CameraSession(CameraManager *cm,
> > @@ -40,13 +42,12 @@ CameraSession::CameraSession(CameraManager *cm,
> >                 camera_ = cm->get(cameraId);
> >
> >         if (!camera_) {
> > -               std::cerr << "Camera " << cameraId << " not found" <<
> > std::endl;
> > +               EPR("Camera {} not found\n", cameraId);
> >                 return;
> >         }
> >
> >         if (camera_->acquire()) {
> > -               std::cerr << "Failed to acquire camera " << cameraId
> > -                         << std::endl;
> > +               EPR("Failed to acquire camera {}", cameraId);
> >                 return;
> >         }
> >
> > @@ -55,15 +56,14 @@ CameraSession::CameraSession(CameraManager *cm,
> >         std::unique_ptr<CameraConfiguration> config =
> >                 camera_->generateConfiguration(roles);
> >         if (!config || config->size() != roles.size()) {
> > -               std::cerr << "Failed to get default stream configuration"
> > -                         << std::endl;
> > +               EPR("Failed to get default stream configuration\n");
> >                 return;
> >         }
> >
> >         /* Apply configuration if explicitly requested. */
> >         if (StreamKeyValueParser::updateConfiguration(config.get(),
> >
> > options_[OptStream])) {
> > -               std::cerr << "Failed to update configuration" << std::endl;
> > +               EPR("Failed to update configuration\n");
> >                 return;
> >         }
> >
> > @@ -72,20 +72,17 @@ CameraSession::CameraSession(CameraManager *cm,
> >  #ifdef HAVE_KMS
> >         if (options_.isSet(OptDisplay)) {
> >                 if (options_.isSet(OptFile)) {
> > -                       std::cerr << "--display and --file options are
> > mutually exclusive"
> > -                                 << std::endl;
> > +                       EPR("--display and --file options are mutually
> > exclusive\n");
> >                         return;
> >                 }
> >
> >                 if (roles.size() != 1) {
> > -                       std::cerr << "Display doesn't support multiple
> > streams"
> > -                                 << std::endl;
> > +                       EPR("Display doesn't support multiple streams\n");
> >                         return;
> >                 }
> >
> >                 if (roles[0] != StreamRole::Viewfinder) {
> > -                       std::cerr << "Display requires a viewfinder stream"
> > -                                 << std::endl;
> > +                       EPR("Display requires a viewfinder stream\n");
> >                         return;
> >                 }
> >         }
> > @@ -97,15 +94,14 @@ CameraSession::CameraSession(CameraManager *cm,
> >
> >         case CameraConfiguration::Adjusted:
> >                 if (strictFormats) {
> > -                       std::cout << "Adjusting camera configuration
> > disallowed by --strict-formats argument"
> > -                                 << std::endl;
> > +                       PR("Adjusting camera configuration disallowed by
> > --strict-formats argument\n");
> >                         return;
> >                 }
> > -               std::cout << "Camera configuration adjusted" << std::endl;
> > +               PR("Camera configuration adjusted\n");
> >                 break;
> >
> >         case CameraConfiguration::Invalid:
> > -               std::cout << "Camera configuration invalid" << std::endl;
> > +               PR("Camera configuration invalid\n");
> >                 return;
> >         }
> >
> > @@ -121,8 +117,7 @@ CameraSession::~CameraSession()
> >  void CameraSession::listControls() const
> >  {
> >         for (const auto &[id, info] : camera_->controls()) {
> > -               std::cout << "Control: " << id->name() << ": "
> > -                         << info.toString() << std::endl;
> > +               PR("Control: {}: {}}n", id->name(), info.toString());
> >         }
> >  }
> >
> > @@ -131,8 +126,7 @@ void CameraSession::listProperties() const
> >         for (const auto &[key, value] : camera_->properties()) {
> >                 const ControlId *id = properties::properties.at(key);
> >
> > -               std::cout << "Property: " << id->name() << " = "
> > -                         << value.toString() << std::endl;
> > +               PR("Property: {} = {}\n", id->name(), value.toString());
> >         }
> >  }
> >
> > @@ -140,17 +134,15 @@ void CameraSession::infoConfiguration() const
> >  {
> >         unsigned int index = 0;
> >         for (const StreamConfiguration &cfg : *config_) {
> > -               std::cout << index << ": " << cfg.toString() << std::endl;
> > +               PR("{}: {}\n", index, cfg.toString());
> >
> >                 const StreamFormats &formats = cfg.formats();
> >                 for (PixelFormat pixelformat : formats.pixelformats()) {
> > -                       std::cout << " * Pixelformat: "
> > -                                 << pixelformat << " "
> > -                                 << formats.range(pixelformat).toString()
> > -                                 << std::endl;
> > +                       PR(" * Pixelformat: {} {}\n", pixelformat,
> > +                          formats.range(pixelformat).toString());
> >
> >                         for (const Size &size : formats.sizes(pixelformat))
> > -                               std::cout << "  - " << size << std::endl;
> > +                               PR("  - {}\n", size);
> >                 }
> >
> >                 index++;
> > @@ -168,7 +160,7 @@ int CameraSession::start()
> >
> >         ret = camera_->configure(config_.get());
> >         if (ret < 0) {
> > -               std::cout << "Failed to configure camera" << std::endl;
> > +               PR("Failed to configure camera\n");
> >                 return ret;
> >         }
> >
> > @@ -197,8 +189,7 @@ int CameraSession::start()
> >         if (sink_) {
> >                 ret = sink_->configure(*config_);
> >                 if (ret < 0) {
> > -                       std::cout << "Failed to configure frame sink"
> > -                                 << std::endl;
> > +                       PR("Failed to configure frame sink\n");
> >                         return ret;
> >                 }
> >
> > @@ -214,12 +205,12 @@ void CameraSession::stop()
> >  {
> >         int ret = camera_->stop();
> >         if (ret)
> > -               std::cout << "Failed to stop capture" << std::endl;
> > +               PR("Failed to stop capture\n");
> >
> >         if (sink_) {
> >                 ret = sink_->stop();
> >                 if (ret)
> > -                       std::cout << "Failed to stop frame sink" <<
> > std::endl;
> > +                       PR("Failed to stop frame sink\n");
> >         }
> >
> >         sink_.reset();
> > @@ -238,7 +229,7 @@ int CameraSession::startCapture()
> >         for (StreamConfiguration &cfg : *config_) {
> >                 ret = allocator_->allocate(cfg.stream());
> >                 if (ret < 0) {
> > -                       std::cerr << "Can't allocate buffers" << std::endl;
> > +                       EPR("Can't allocate buffers\n");
> >                         return -ENOMEM;
> >                 }
> >
> > @@ -254,7 +245,7 @@ int CameraSession::startCapture()
> >         for (unsigned int i = 0; i < nbuffers; i++) {
> >                 std::unique_ptr<Request> request =
> > camera_->createRequest();
> >                 if (!request) {
> > -                       std::cerr << "Can't create request" << std::endl;
> > +                       EPR("Can't create request\n");
> >                         return -ENOMEM;
> >                 }
> >
> > @@ -266,8 +257,7 @@ int CameraSession::startCapture()
> >
> >                         ret = request->addBuffer(stream, buffer.get());
> >                         if (ret < 0) {
> > -                               std::cerr << "Can't set buffer for request"
> > -                                         << std::endl;
> > +                               EPR("Can't set buffer for request\n");
> >                                 return ret;
> >                         }
> >
> > @@ -281,14 +271,14 @@ int CameraSession::startCapture()
> >         if (sink_) {
> >                 ret = sink_->start();
> >                 if (ret) {
> > -                       std::cout << "Failed to start frame sink" <<
> > std::endl;
> > +                       PR("Failed to start frame sink\n");
> >                         return ret;
> >                 }
> >         }
> >
> >         ret = camera_->start();
> >         if (ret) {
> > -               std::cout << "Failed to start capture" << std::endl;
> > +               PR("Failed to start capture\n");
> >                 if (sink_)
> >                         sink_->stop();
> >                 return ret;
> > @@ -297,7 +287,7 @@ int CameraSession::startCapture()
> >         for (std::unique_ptr<Request> &request : requests_) {
> >                 ret = queueRequest(request.get());
> >                 if (ret < 0) {
> > -                       std::cerr << "Can't queue request" << std::endl;
> > +                       EPR("Can't queue request\n");
> >                         camera_->stop();
> >                         if (sink_)
> >                                 sink_->stop();
> > @@ -306,13 +296,11 @@ int CameraSession::startCapture()
> >         }
> >
> >         if (captureLimit_)
> > -               std::cout << "cam" << cameraIndex_
> > -                         << ": Capture " << captureLimit_ << " frames"
> > -                         << std::endl;
> > +               PR("cam{}: Capture {} frames\n", cameraIndex_,
> > +                  captureLimit_);
> >         else
> > -               std::cout << "cam" << cameraIndex_
> > -                         << ": Capture until user interrupts by SIGINT"
> > -                         << std::endl;
> > +               PR("cam{}: Capture until user interrupts by SIGINT\n",
> > +                  cameraIndex_);
> >
> >         return 0;
> >  }
> > @@ -364,23 +352,23 @@ void CameraSession::processRequest(Request *request)
> >
> >         bool requeue = true;
> >
> > -       std::stringstream info;
> > -       info << ts / 1000000000 << "."
> > -            << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
> > -            << " (" << std::fixed << std::setprecision(2) << fps << "
> > fps)";
> > +       auto sbuf = fmt::memory_buffer();
> > +       fmt::format_to(std::back_inserter(sbuf), "{}.{:06} ({:.2f} fps)",
> > +                      ts / 1000000000,
> > +                      ts / 1000 % 1000000,
> > +                      fps);
> >
> >         for (const auto &[stream, buffer] : buffers) {
> >                 const FrameMetadata &metadata = buffer->metadata();
> >
> > -               info << " " << streamNames_[stream]
> > -                    << " seq: " << std::setw(6) << std::setfill('0') <<
> > metadata.sequence
> > -                    << " bytesused: ";
> > +               fmt::format_to(std::back_inserter(sbuf), " {} seq: {:06}
> > bytesused: ",
> > +                              streamNames_[stream], metadata.sequence);
> >
> >                 unsigned int nplane = 0;
> >                 for (const FrameMetadata::Plane &plane :
> > metadata.planes()) {
> > -                       info << plane.bytesused;
> > +                       fmt::format_to(std::back_inserter(sbuf), "{}",
> > plane.bytesused);
> >                         if (++nplane < metadata.planes().size())
> > -                               info << "/";
> > +                               fmt::format_to(std::back_inserter(sbuf),
> > "/");
> >                 }
> >         }
> >
> > @@ -389,14 +377,13 @@ void CameraSession::processRequest(Request *request)
> >                         requeue = false;
> >         }
> >
> > -       std::cout << info.str() << std::endl;
> > +       PR("{}\n", fmt::to_string(sbuf));
> >
> >         if (printMetadata_) {
> >                 const ControlList &requestMetadata = request->metadata();
> >                 for (const auto &[key, value] : requestMetadata) {
> >                         const ControlId *id = controls::controls.at(key);
> > -                       std::cout << "\t" << id->name() << " = "
> > -                                 << value.toString() << std::endl;
> > +                       PR("\t{} = {}\n", id->name(), value.toString());
> >                 }
> >         }
> >
> > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > index 46e34eb5..84919ab3 100644
> > --- a/src/cam/drm.cpp
> > +++ b/src/cam/drm.cpp
> > @@ -10,12 +10,12 @@
> >  #include <algorithm>
> >  #include <errno.h>
> >  #include <fcntl.h>
> > -#include <iostream>
> >  #include <set>
> >  #include <string.h>
> >  #include <sys/ioctl.h>
> >  #include <sys/stat.h>
> >  #include <sys/types.h>
> > +#include <fmt/core.h>
> >
> >  #include <libcamera/framebuffer.h>
> >  #include <libcamera/geometry.h>
> > @@ -25,6 +25,9 @@
> >
> >  #include "event_loop.h"
> >
> > +#define PR(...) fmt::print(__VA_ARGS__)
> > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> > +
> >  namespace DRM {
> >
> >  Object::Object(Device *dev, uint32_t id, Type type)
> > @@ -178,9 +181,7 @@ Connector::Connector(Device *dev, const
> > drmModeConnector *connector)
> >  {
> >         auto typeName = connectorTypeNames.find(connector->connector_type);
> >         if (typeName == connectorTypeNames.end()) {
> > -               std::cerr
> > -                       << "Invalid connector type "
> > -                       << connector->connector_type << std::endl;
> > +               EPR("Invalid connector type {}}n",
> > connector->connector_type);
> >                 typeName =
> > connectorTypeNames.find(DRM_MODE_CONNECTOR_Unknown);
> >         }
> >
> > @@ -213,9 +214,7 @@ Connector::Connector(Device *dev, const
> > drmModeConnector *connector)
> >                                                     return e.id() ==
> > encoderId;
> >                                             });
> >                 if (encoder == encoders.end()) {
> > -                       std::cerr
> > -                               << "Encoder " << encoderId << " not found"
> > -                               << std::endl;
> > +                       EPR("Encoder {} not found\n", encoderId);
> >                         continue;
> >                 }
> >
> > @@ -296,9 +295,7 @@ FrameBuffer::~FrameBuffer()
> >
> >                 if (ret == -1) {
> >                         ret = -errno;
> > -                       std::cerr
> > -                               << "Failed to close GEM object: "
> > -                               << strerror(-ret) << std::endl;
> > +                       EPR("Failed to close GEM object: {}\n",
> > strerror(-ret));
> >                 }
> >         }
> >
> > @@ -408,9 +405,8 @@ int Device::init()
> >         fd_ = open(name, O_RDWR | O_CLOEXEC);
> >         if (fd_ < 0) {
> >                 ret = -errno;
> > -               std::cerr
> > -                       << "Failed to open DRM/KMS device " << name << ": "
> > -                       << strerror(-ret) << std::endl;
> > +               EPR("Failed to open DRM/KMS device {}: {}\n", name,
> > +                   strerror(-ret));
> >                 return ret;
> >         }
> >
> > @@ -421,9 +417,7 @@ int Device::init()
> >         ret = drmSetClientCap(fd_, DRM_CLIENT_CAP_ATOMIC, 1);
> >         if (ret < 0) {
> >                 ret = -errno;
> > -               std::cerr
> > -                       << "Failed to enable atomic capability: "
> > -                       << strerror(-ret) << std::endl;
> > +               EPR("Failed to enable atomic capability: {}\n",
> > strerror(-ret));
> >                 return ret;
> >         }
> >
> > @@ -448,9 +442,7 @@ int Device::getResources()
> >         };
> >         if (!resources) {
> >                 ret = -errno;
> > -               std::cerr
> > -                       << "Failed to get DRM/KMS resources: "
> > -                       << strerror(-ret) << std::endl;
> > +               EPR("Failed to get DRM/KMS resources: {}\n",
> > strerror(-ret));
> >                 return ret;
> >         }
> >
> > @@ -458,9 +450,7 @@ int Device::getResources()
> >                 drmModeCrtc *crtc = drmModeGetCrtc(fd_,
> > resources->crtcs[i]);
> >                 if (!crtc) {
> >                         ret = -errno;
> > -                       std::cerr
> > -                               << "Failed to get CRTC: " << strerror(-ret)
> > -                               << std::endl;
> > +                       EPR("Failed to get CRTC: {}\n", strerror(-ret));
> >                         return ret;
> >                 }
> >
> > @@ -476,9 +466,7 @@ int Device::getResources()
> >                         drmModeGetEncoder(fd_, resources->encoders[i]);
> >                 if (!encoder) {
> >                         ret = -errno;
> > -                       std::cerr
> > -                               << "Failed to get encoder: " <<
> > strerror(-ret)
> > -                               << std::endl;
> > +                       EPR("Failed to get encoder: {}\n", strerror(-ret));
> >                         return ret;
> >                 }
> >
> > @@ -494,9 +482,7 @@ int Device::getResources()
> >                         drmModeGetConnector(fd_, resources->connectors[i]);
> >                 if (!connector) {
> >                         ret = -errno;
> > -                       std::cerr
> > -                               << "Failed to get connector: " <<
> > strerror(-ret)
> > -                               << std::endl;
> > +                       EPR("Failed to get connector: {}\n",
> > strerror(-ret));
> >                         return ret;
> >                 }
> >
> > @@ -513,9 +499,7 @@ int Device::getResources()
> >         };
> >         if (!planes) {
> >                 ret = -errno;
> > -               std::cerr
> > -                       << "Failed to get DRM/KMS planes: "
> > -                       << strerror(-ret) << std::endl;
> > +               EPR("Failed to get DRM/KMS planes: {}\n", strerror(-ret));
> >                 return ret;
> >         }
> >
> > @@ -524,9 +508,7 @@ int Device::getResources()
> >                         drmModeGetPlane(fd_, planes->planes[i]);
> >                 if (!plane) {
> >                         ret = -errno;
> > -                       std::cerr
> > -                               << "Failed to get plane: " <<
> > strerror(-ret)
> > -                               << std::endl;
> > +                       EPR("Failed to get plane: {}\n", strerror(-ret));
> >                         return ret;
> >                 }
> >
> > @@ -556,9 +538,7 @@ int Device::getResources()
> >                 drmModePropertyRes *property = drmModeGetProperty(fd_, id);
> >                 if (!property) {
> >                         ret = -errno;
> > -                       std::cerr
> > -                               << "Failed to get property: " <<
> > strerror(-ret)
> > -                               << std::endl;
> > +                       EPR("Failed to get property: {}\n",
> > strerror(-ret));
> >                         continue;
> >                 }
> >
> > @@ -573,9 +553,8 @@ int Device::getResources()
> >         for (auto &object : objects_) {
> >                 ret = object.second->setup();
> >                 if (ret < 0) {
> > -                       std::cerr
> > -                               << "Failed to setup object " <<
> > object.second->id()
> > -                               << ": " << strerror(-ret) << std::endl;
> > +                       EPR("Failed to setup object {}: {}\n",
> > +                           object.second->id(), strerror(-ret));
> >                         return ret;
> >                 }
> >         }
> > @@ -616,9 +595,8 @@ std::unique_ptr<FrameBuffer> Device::createFrameBuffer(
> >                         ret = drmPrimeFDToHandle(fd_, plane.fd.get(),
> > &handle);
> >                         if (ret < 0) {
> >                                 ret = -errno;
> > -                               std::cerr
> > -                                       << "Unable to import framebuffer
> > dmabuf: "
> > -                                       << strerror(-ret) << std::endl;
> > +                               EPR("Unable to import framebuffer dmabuf:
> > {}\n",
> > +                                   strerror(-ret));
> >                                 return nullptr;
> >                         }
> >
> > @@ -636,9 +614,7 @@ std::unique_ptr<FrameBuffer> Device::createFrameBuffer(
> >                             strides.data(), offsets, &fb->id_, 0);
> >         if (ret < 0) {
> >                 ret = -errno;
> > -               std::cerr
> > -                       << "Failed to add framebuffer: "
> > -                       << strerror(-ret) << std::endl;
> > +               EPR("Failed to add framebuffer: {}\n", strerror(-ret));
> >                 return nullptr;
> >         }
> >
> > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > index e25784c0..87aaf59a 100644
> > --- a/src/cam/event_loop.cpp
> > +++ b/src/cam/event_loop.cpp
> > @@ -10,7 +10,10 @@
> >  #include <assert.h>
> >  #include <event2/event.h>
> >  #include <event2/thread.h>
> > -#include <iostream>
> > +#include <fmt/core.h>
> > +
> > +#define PR(...) fmt::print(__VA_ARGS__)
> > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> >
> >  EventLoop *EventLoop::instance_ = nullptr;
> >
> > @@ -71,13 +74,13 @@ void EventLoop::addEvent(int fd, EventType type,
> >         event->event_ = event_new(base_, fd, events,
> > &EventLoop::Event::dispatch,
> >                                   event.get());
> >         if (!event->event_) {
> > -               std::cerr << "Failed to create event for fd " << fd <<
> > std::endl;
> > +               EPR("Failed to create event for fd {}\n", fd);
> >                 return;
> >         }
> >
> >         int ret = event_add(event->event_, nullptr);
> >         if (ret < 0) {
> > -               std::cerr << "Failed to add event for fd " << fd <<
> > std::endl;
> > +               EPR("Failed to add event for fd {}\n", fd);
> >                 return;
> >         }
> >
> > diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp
> > index 45213d4a..86e2118c 100644
> > --- a/src/cam/file_sink.cpp
> > +++ b/src/cam/file_sink.cpp
> > @@ -7,11 +7,12 @@
> >
> >  #include <assert.h>
> >  #include <fcntl.h>
> > -#include <iomanip>
> > -#include <iostream>
> > -#include <sstream>
> >  #include <string.h>
> >  #include <unistd.h>
> > +#include <fmt/core.h>
> > +
> > +#define PR(...) fmt::print(__VA_ARGS__)
> > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> >
> >  #include <libcamera/camera.h>
> >
> > @@ -70,10 +71,10 @@ void FileSink::writeBuffer(const Stream *stream,
> > FrameBuffer *buffer)
> >
> >         pos = filename.find_first_of('#');
> >         if (pos != std::string::npos) {
> > -               std::stringstream ss;
> > -               ss << streamNames_[stream] << "-" << std::setw(6)
> > -                  << std::setfill('0') << buffer->metadata().sequence;
> > -               filename.replace(pos, 1, ss.str());
> > +               auto s = fmt::format("{}-{:06}",
> > +                                    streamNames_[stream],
> > +                                    buffer->metadata().sequence);
> > +               filename.replace(pos, 1, s);
> >         }
> >
> >         fd = open(filename.c_str(), O_CREAT | O_WRONLY |
> > @@ -81,8 +82,7 @@ void FileSink::writeBuffer(const Stream *stream,
> > FrameBuffer *buffer)
> >                   S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH |
> > S_IWOTH);
> >         if (fd == -1) {
> >                 ret = -errno;
> > -               std::cerr << "failed to open file " << filename << ": "
> > -                         << strerror(-ret) << std::endl;
> > +               EPR("failed to open file {}: {}\n", filename,
> > strerror(-ret));
> >                 return;
> >         }
> >
> > @@ -95,20 +95,17 @@ void FileSink::writeBuffer(const Stream *stream,
> > FrameBuffer *buffer)
> >                 unsigned int length = std::min<unsigned
> > int>(meta.bytesused, data.size());
> >
> >                 if (meta.bytesused > data.size())
> > -                       std::cerr << "payload size " << meta.bytesused
> > -                                 << " larger than plane size " <<
> > data.size()
> > -                                 << std::endl;
> > +                       EPR("payload size {} larger than plane size {}\n",
> > +                           meta.bytesused, data.size());
> >
> >                 ret = ::write(fd, data.data(), length);
> >                 if (ret < 0) {
> >                         ret = -errno;
> > -                       std::cerr << "write error: " << strerror(-ret)
> > -                                 << std::endl;
> > +                       EPR("write error: {}\n", strerror(-ret));
> >                         break;
> >                 } else if (ret != (int)length) {
> > -                       std::cerr << "write error: only " << ret
> > -                                 << " bytes written instead of "
> > -                                 << length << std::endl;
> > +                       EPR("write error: only {} bytes written instead of
> > {}\n",
> > +                           ret, length);
> >                         break;
> >                 }
> >         }
> > diff --git a/src/cam/image.cpp b/src/cam/image.cpp
> > index fe2cc6da..73bcf915 100644
> > --- a/src/cam/image.cpp
> > +++ b/src/cam/image.cpp
> > @@ -9,11 +9,14 @@
> >
> >  #include <assert.h>
> >  #include <errno.h>
> > -#include <iostream>
> >  #include <map>
> >  #include <string.h>
> >  #include <sys/mman.h>
> >  #include <unistd.h>
> > +#include <fmt/core.h>
> > +
> > +#define PR(...) fmt::print(__VA_ARGS__)
> > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> >
> >  using namespace libcamera;
> >
> > @@ -49,10 +52,8 @@ std::unique_ptr<Image> Image::fromFrameBuffer(const
> > FrameBuffer *buffer, MapMode
> >
> >                 if (plane.offset > length ||
> >                     plane.offset + plane.length > length) {
> > -                       std::cerr << "plane is out of buffer: buffer
> > length="
> > -                                 << length << ", plane offset=" <<
> > plane.offset
> > -                                 << ", plane length=" << plane.length
> > -                                 << std::endl;
> > +                       EPR("plane is out of buffer: buffer length={},
> > plane offset={}, plane length={}\n",
> > +                           length, plane.offset, plane.length);
> >                         return nullptr;
> >                 }
> >                 size_t &mapLength = mappedBuffers[fd].mapLength;
> > @@ -68,8 +69,8 @@ std::unique_ptr<Image> Image::fromFrameBuffer(const
> > FrameBuffer *buffer, MapMode
> >                                              MAP_SHARED, fd, 0);
> >                         if (address == MAP_FAILED) {
> >                                 int error = -errno;
> > -                               std::cerr << "Failed to mmap plane: "
> > -                                         << strerror(-error) << std::endl;
> > +                               EPR("Failed to mmap plane: {}\n",
> > +                                   strerror(-error));
> >                                 return nullptr;
> >                         }
> >
> > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > index 7add81a6..823b75e4 100644
> > --- a/src/cam/kms_sink.cpp
> > +++ b/src/cam/kms_sink.cpp
> > @@ -10,10 +10,13 @@
> >  #include <array>
> >  #include <algorithm>
> >  #include <assert.h>
> > -#include <iostream>
> >  #include <memory>
> >  #include <stdint.h>
> >  #include <string.h>
> > +#include <fmt/core.h>
> > +
> > +#define PR(...) fmt::print(__VA_ARGS__)
> > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> >
> >  #include <libcamera/camera.h>
> >  #include <libcamera/formats.h>
> > @@ -54,11 +57,9 @@ KMSSink::KMSSink(const std::string &connectorName)
> >
> >         if (!connector_) {
> >                 if (!connectorName.empty())
> > -                       std::cerr
> > -                               << "Connector " << connectorName << " not
> > found"
> > -                               << std::endl;
> > +                       EPR("Connector {} not found\n", connectorName);
> >                 else
> > -                       std::cerr << "No connected connector found" <<
> > std::endl;
> > +                       EPR("No connected connector found\n");
> >                 return;
> >         }
> >
> > @@ -119,7 +120,7 @@ int KMSSink::configure(const
> > libcamera::CameraConfiguration &config)
> >                                                       mode.vdisplay ==
> > cfg.size.height;
> >                                        });
> >         if (iter == modes.end()) {
> > -               std::cerr << "No mode matching " << cfg.size << std::endl;
> > +               EPR("No mode matching {}\n", cfg.size);
> >                 return -EINVAL;
> >         }
> >
> > @@ -192,17 +193,12 @@ int KMSSink::configurePipeline(const
> > libcamera::PixelFormat &format)
> >  {
> >         const int ret = selectPipeline(format);
> >         if (ret) {
> > -               std::cerr
> > -                       << "Unable to find display pipeline for format "
> > -                       << format << std::endl;
> > -
> > +               EPR("Unable to find display pipeline for format {}\n",
> > format);
> >                 return ret;
> >         }
> >
> > -       std::cout
> > -               << "Using KMS plane " << plane_->id() << ", CRTC " <<
> > crtc_->id()
> > -               << ", connector " << connector_->name()
> > -               << " (" << connector_->id() << ")" << std::endl;
> > +       PR("Using KMS plane {}, CRTC {}, connector {} ({})\n",
> > +          plane_->id(), crtc_->id(), connector_->name(),
> > connector_->id());
> >
> >         return 0;
> >  }
> > @@ -228,9 +224,8 @@ int KMSSink::start()
> >
> >         ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> >         if (ret < 0) {
> > -               std::cerr
> > -                       << "Failed to disable CRTCs and planes: "
> > -                       << strerror(-ret) << std::endl;
> > +               EPR("Failed to disable CRTCs and planes: {}\n",
> > +                   strerror(-ret));
> >                 return ret;
> >         }
> >
> > @@ -250,9 +245,7 @@ int KMSSink::stop()
> >
> >         int ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);
> >         if (ret < 0) {
> > -               std::cerr
> > -                       << "Failed to stop display pipeline: "
> > -                       << strerror(-ret) << std::endl;
> > +               EPR("Failed to stop display pipeline: {}\n",
> > strerror(-ret));
> >                 return ret;
> >         }
> >
> > @@ -312,9 +305,8 @@ bool KMSSink::processRequest(libcamera::Request
> > *camRequest)
> >         if (!queued_) {
> >                 int ret = drmRequest->commit(flags);
> >                 if (ret < 0) {
> > -                       std::cerr
> > -                               << "Failed to commit atomic request: "
> > -                               << strerror(-ret) << std::endl;
> > +                       EPR("Failed to commit atomic request: {}\n",
> > +                           strerror(-ret));
> >                         /* \todo Implement error handling */
> >                 }
> >
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index c7f664b9..03615dc9 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -6,10 +6,9 @@
> >   */
> >
> >  #include <atomic>
> > -#include <iomanip>
> > -#include <iostream>
> >  #include <signal.h>
> >  #include <string.h>
> > +#include <fmt/core.h>
> >
> >  #include <libcamera/libcamera.h>
> >  #include <libcamera/property_ids.h>
> > @@ -78,8 +77,7 @@ int CamApp::init(int argc, char **argv)
> >
> >         ret = cm_->start();
> >         if (ret) {
> > -               std::cout << "Failed to start camera manager: "
> > -                         << strerror(-ret) << std::endl;
> > +               fmt::print("Failed to start camera manager: {}\n", -ret);
> >                 return ret;
> >         }
> >
> > @@ -173,12 +171,12 @@ int CamApp::parseOptions(int argc, char *argv[])
> >
> >  void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
> >  {
> > -       std::cout << "Camera Added: " << cam->id() << std::endl;
> > +       fmt::print("Camera Added: {}\n", cam->id());
> >  }
> >
> >  void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
> >  {
> > -       std::cout << "Camera Removed: " << cam->id() << std::endl;
> > +       fmt::print("Camera Removed: {}\n", cam->id());
> >  }
> >
> >  void CamApp::captureDone()
> > @@ -193,11 +191,11 @@ int CamApp::run()
> >
> >         /* 1. List all cameras. */
> >         if (options_.isSet(OptList)) {
> > -               std::cout << "Available cameras:" << std::endl;
> > +               fmt::print("Available cameras:\n");
> >
> >                 unsigned int index = 1;
> >                 for (const std::shared_ptr<Camera> &cam : cm_->cameras()) {
> > -                       std::cout << index << ": " <<
> > cameraName(cam.get()) << std::endl;
> > +                       fmt::print("{}: {}\n", cameraName(cam.get()),
> > index);
> >                         index++;
> >                 }
> >         }
> > @@ -215,12 +213,12 @@ int CamApp::run()
> >                                                                 index,
> >
> > camera.children());
> >                         if (!session->isValid()) {
> > -                               std::cout << "Failed to create camera
> > session" << std::endl;
> > +                               fmt::print("Failed to create camera
> > session\n");
> >                                 return -EINVAL;
> >                         }
> >
> > -                       std::cout << "Using camera " <<
> > session->camera()->id()
> > -                                 << " as cam" << index << std::endl;
> > +                       fmt::print("Using camera{} as cam{}\n",
> > +                                  session->camera()->id(), index);
> >
> >                         session->captureDone.connect(this,
> > &CamApp::captureDone);
> >
> > @@ -250,7 +248,7 @@ int CamApp::run()
> >
> >                 ret = session->start();
> >                 if (ret) {
> > -                       std::cout << "Failed to start camera session" <<
> > std::endl;
> > +                       fmt::print("Failed to start camera session\n");
> >                         return ret;
> >                 }
> >
> > @@ -259,8 +257,8 @@ int CamApp::run()
> >
> >         /* 5. Enable hotplug monitoring. */
> >         if (options_.isSet(OptMonitor)) {
> > -               std::cout << "Monitoring new hotplug and unplug events" <<
> > std::endl;
> > -               std::cout << "Press Ctrl-C to interrupt" << std::endl;
> > +               fmt::print("Monitoring new hotplug and unplug events\n");
> > +               fmt::print("Press Ctrl-C to interrupt\n");
> >
> >                 cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> >                 cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> > @@ -323,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)
> >
> >  void signalHandler([[maybe_unused]] int signal)
> >  {
> > -       std::cout << "Exiting" << std::endl;
> > +       fmt::print("Exiting");
> >         CamApp::instance()->quit();
> >  }
> >
> > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > index 5bab8c9e..2b47383d 100644
> > --- a/src/cam/meson.build
> > +++ b/src/cam/meson.build
> > @@ -7,6 +7,8 @@ if not libevent.found()
> >      subdir_done()
> >  endif
> >
> > +libfmt_dep = dependency('fmt')
> > +
> >  cam_enabled = true
> >
> >  cam_sources = files([
> > @@ -25,7 +27,7 @@ cam_cpp_args = []
> >  libdrm = dependency('libdrm', required : false)
> >
> >  if libdrm.found()
> > -    cam_cpp_args += [ '-DHAVE_KMS' ]
> > +    cam_cpp_args += [ '-DHAVE_KMS', ]
> >      cam_sources += files([
> >          'drm.cpp',
> >          'kms_sink.cpp'
> > @@ -38,6 +40,7 @@ cam  = executable('cam', cam_sources,
> >                        libcamera_public,
> >                        libdrm,
> >                        libevent,
> > +                      libfmt_dep,
> >                    ],
> >                    cpp_args : cam_cpp_args,
> >                    install : true)
> > diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> > index 4f7e8691..c9979385 100644
> > --- a/src/cam/options.cpp
> > +++ b/src/cam/options.cpp
> > @@ -7,9 +7,11 @@
> >
> >  #include <assert.h>
> >  #include <getopt.h>
> > -#include <iomanip>
> > -#include <iostream>
> >  #include <string.h>
> > +#include <fmt/core.h>
> > +
> > +#define PR(...) fmt::print(__VA_ARGS__)
> > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> >
> >  #include "options.h"
> >
> > @@ -390,26 +392,23 @@ KeyValueParser::Options KeyValueParser::parse(const
> > char *arguments)
> >                         continue;
> >
> >                 if (optionsMap_.find(key) == optionsMap_.end()) {
> > -                       std::cerr << "Invalid option " << key << std::endl;
> > +                       EPR("Invalid option {}\n", key);
> >                         return options;
> >                 }
> >
> >                 OptionArgument arg = optionsMap_[key].argument;
> >                 if (value.empty() && arg == ArgumentRequired) {
> > -                       std::cerr << "Option " << key << " requires an
> > argument"
> > -                                 << std::endl;
> > +                       EPR("Option {} requires an argument\n", key);
> >                         return options;
> >                 } else if (!value.empty() && arg == ArgumentNone) {
> > -                       std::cerr << "Option " << key << " takes no
> > argument"
> > -                                 << std::endl;
> > +                       EPR("Option {} takes no argument\n", key);
> >                         return options;
> >                 }
> >
> >                 const Option &option = optionsMap_[key];
> >                 if (!options.parseValue(key, option, value.c_str())) {
> > -                       std::cerr << "Failed to parse '" << value << "' as
> > "
> > -                                 << option.typeName() << " for option "
> > << key
> > -                                 << std::endl;
> > +                       EPR("Failed to parse '{}' as {} for option {}\n",
> > +                           value, option.typeName(), key);
> >                         return options;
> >                 }
> >         }
> > @@ -453,16 +452,16 @@ void KeyValueParser::usage(int indent)
> >                                 argument += "]";
> >                 }
> >
> > -               std::cerr << std::setw(indent) << argument;
> > +               EPR("{:{}}", argument, indent);
> >
> >                 for (const char *help = option.help, *end = help; end;) {
> >                         end = strchr(help, '\n');
> >                         if (end) {
> > -                               std::cerr << std::string(help, end - help
> > + 1);
> > -                               std::cerr << std::setw(indent) << " ";
> > +                               EPR(std::string(help, end - help + 1));
> > +                               EPR("{:{}}", "", indent);
> >                                 help = end + 1;
> >                         } else {
> > -                               std::cerr << help << std::endl;
> > +                               EPR("{}\n", help);
> >                         }
> >                 }
> >         }
> > @@ -929,10 +928,10 @@ OptionsParser::Options OptionsParser::parse(int
> > argc, char **argv)
> >
> >                 if (c == '?' || c == ':') {
> >                         if (c == '?')
> > -                               std::cerr << "Invalid option ";
> > +                               EPR("Invalid option ");
> >                         else
> > -                               std::cerr << "Missing argument for option
> > ";
> > -                       std::cerr << argv[optind - 1] << std::endl;
> > +                               EPR("Missing argument for option ");
> > +                       EPR("{}\n", argv[optind - 1]);
> >
> >                         usage();
> >                         return options;
> > @@ -946,8 +945,7 @@ OptionsParser::Options OptionsParser::parse(int argc,
> > char **argv)
> >         }
> >
> >         if (optind < argc) {
> > -               std::cerr << "Invalid non-option argument '" <<
> > argv[optind]
> > -                         << "'" << std::endl;
> > +               EPR("Invalid non-option argument '{}'\n", argv[optind]);
> >                 usage();
> >                 return options;
> >         }
> > @@ -992,14 +990,9 @@ void OptionsParser::usage()
> >
> >         indent = (indent + 7) / 8 * 8;
> >
> > -       std::cerr << "Options:" << std::endl;
> > -
> > -       std::ios_base::fmtflags f(std::cerr.flags());
> > -       std::cerr << std::left;
> > +       EPR("Options:\n");
> >
> >         usageOptions(options_, indent);
> > -
> > -       std::cerr.flags(f);
> >  }
> >
> >  void OptionsParser::usageOptions(const std::list<Option> &options,
> > @@ -1036,16 +1029,16 @@ void OptionsParser::usageOptions(const
> > std::list<Option> &options,
> >                 if (option.isArray)
> >                         argument += " ...";
> >
> > -               std::cerr << std::setw(indent) << argument;
> > +               EPR("{:{}}", argument, indent);
> >
> > -               for (const char *help = option.help, *end = help; end; ) {
> > +               for (const char *help = option.help, *end = help; end;) {
> >                         end = strchr(help, '\n');
> >                         if (end) {
> > -                               std::cerr << std::string(help, end - help
> > + 1);
> > -                               std::cerr << std::setw(indent) << " ";
> > +                               EPR(std::string(help, end - help + 1));
> > +                               EPR("{:{}}", "", indent);
> >                                 help = end + 1;
> >                         } else {
> > -                               std::cerr << help << std::endl;
> > +                               EPR("{}\n", help);
> >                         }
> >                 }
> >
> > @@ -1060,8 +1053,8 @@ void OptionsParser::usageOptions(const
> > std::list<Option> &options,
> >                 return;
> >
> >         for (const Option *option : parentOptions) {
> > -               std::cerr << std::endl << "Options valid in the context of
> > "
> > -                         << option->optionName() << ":" << std::endl;
> > +               EPR("\nOptions valid in the context of {}:\n",
> > +                   option->optionName());
> >                 usageOptions(option->children, indent);
> >         }
> >  }
> > @@ -1125,15 +1118,14 @@ bool OptionsParser::parseValue(const Option
> > &option, const char *arg,
> >
> >         std::tie(options, error) = childOption(option.parent, options);
> >         if (error) {
> > -               std::cerr << "Option " << option.optionName() << "
> > requires a "
> > -                         << error->optionName() << " context" <<
> > std::endl;
> > +               EPR("Option {} requires a {} context\n",
> > +                   option.optionName(), error->optionName());
> >                 return false;
> >         }
> >
> >         if (!options->parseValue(option.opt, option, arg)) {
> > -               std::cerr << "Can't parse " << option.typeName()
> > -                         << " argument for option " << option.optionName()
> > -                         << std::endl;
> > +               EPR("Can't parse {} argument for option {}\n",
> > +                   option.typeName(), option.optionName());
> >                 return false;
> >         }
> >
> > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp
> > index 150bd27c..666862eb 100644
> > --- a/src/cam/stream_options.cpp
> > +++ b/src/cam/stream_options.cpp
> > @@ -6,7 +6,10 @@
> >   */
> >  #include "stream_options.h"
> >
> > -#include <iostream>
> > +#include <fmt/core.h>
> > +
> > +#define PR(...) fmt::print(__VA_ARGS__)
> > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> >
> >  using namespace libcamera;
> >
> > @@ -30,8 +33,7 @@ KeyValueParser::Options
> > StreamKeyValueParser::parse(const char *arguments)
> >
> >         if (options.valid() && options.isSet("role") &&
> >             !parseRole(&role, options)) {
> > -               std::cerr << "Unknown stream role "
> > -                         << options["role"].toString() << std::endl;
> > +               EPR("Unknown stream role {}\n",
> > options["role"].toString());
> >                 options.invalidate();
> >         }
> >
> > @@ -64,7 +66,7 @@ int
> > StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
> >                                               const OptionValue &values)
> >  {
> >         if (!config) {
> > -               std::cerr << "No configuration provided" << std::endl;
> > +               EPR("No configuration provided\n");
> >                 return -EINVAL;
> >         }
> >
> > @@ -75,12 +77,8 @@ int
> > StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
> >         const std::vector<OptionValue> &streamParameters =
> > values.toArray();
> >
> >         if (config->size() != streamParameters.size()) {
> > -               std::cerr
> > -                       << "Number of streams in configuration "
> > -                       << config->size()
> > -                       << " does not match number of streams parsed "
> > -                       << streamParameters.size()
> > -                       << std::endl;
> > +               EPR("Number of streams in configuration {} does not match
> > number of streams parsed {}\n",
> > +                   config->size(), streamParameters.size());
> >                 return -EINVAL;
> >         }
> >
> > --
> > 2.34.1
> >
> >

Eric Curtin May 10, 2022, 12:59 p.m. UTC | #4

On Tue, 10 May 2022 at 11:10, Kieran Bingham via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Quoting Naushir Patuck (2022-05-10 10:49:14)
> > Hi Tomi,
> >
> > On Tue, 10 May 2022 at 08:16, Tomi Valkeinen via libcamera-devel <
> > libcamera-devel@lists.libcamera.org> 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@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 :)

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.

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.
>
> --
> Kieran
>
>
> >
> > Regards,
> > Naush
> >
> >
> >
> > > ---
> > >  src/cam/camera_session.cpp | 105 ++++++++++++++++---------------------
> > >  src/cam/drm.cpp            |  68 ++++++++----------------
> > >  src/cam/event_loop.cpp     |   9 ++--
> > >  src/cam/file_sink.cpp      |  31 +++++------
> > >  src/cam/image.cpp          |  15 +++---
> > >  src/cam/kms_sink.cpp       |  38 ++++++--------
> > >  src/cam/main.cpp           |  28 +++++-----
> > >  src/cam/meson.build        |   5 +-
> > >  src/cam/options.cpp        |  66 ++++++++++-------------
> > >  src/cam/stream_options.cpp |  18 +++----
> > >  10 files changed, 165 insertions(+), 218 deletions(-)
> > >
> > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > > index efffafbf..7843c3fd 100644
> > > --- a/src/cam/camera_session.cpp
> > > +++ b/src/cam/camera_session.cpp
> > > @@ -5,10 +5,9 @@
> > >   * camera_session.cpp - Camera capture session
> > >   */
> > >
> > > -#include <iomanip>
> > > -#include <iostream>
> > >  #include <limits.h>
> > > -#include <sstream>
> > > +#include <fmt/format.h>
> > > +#include <fmt/ostream.h>
> > >
> > >  #include <libcamera/control_ids.h>
> > >  #include <libcamera/property_ids.h>
> > > @@ -22,6 +21,9 @@
> > >  #include "main.h"
> > >  #include "stream_options.h"
> > >
> > > +#define PR(...) fmt::print(__VA_ARGS__)
> > > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> > > +
> > >  using namespace libcamera;
> > >
> > >  CameraSession::CameraSession(CameraManager *cm,
> > > @@ -40,13 +42,12 @@ CameraSession::CameraSession(CameraManager *cm,
> > >                 camera_ = cm->get(cameraId);
> > >
> > >         if (!camera_) {
> > > -               std::cerr << "Camera " << cameraId << " not found" <<
> > > std::endl;
> > > +               EPR("Camera {} not found\n", cameraId);
> > >                 return;
> > >         }
> > >
> > >         if (camera_->acquire()) {
> > > -               std::cerr << "Failed to acquire camera " << cameraId
> > > -                         << std::endl;
> > > +               EPR("Failed to acquire camera {}", cameraId);
> > >                 return;
> > >         }
> > >
> > > @@ -55,15 +56,14 @@ CameraSession::CameraSession(CameraManager *cm,
> > >         std::unique_ptr<CameraConfiguration> config =
> > >                 camera_->generateConfiguration(roles);
> > >         if (!config || config->size() != roles.size()) {
> > > -               std::cerr << "Failed to get default stream configuration"
> > > -                         << std::endl;
> > > +               EPR("Failed to get default stream configuration\n");
> > >                 return;
> > >         }
> > >
> > >         /* Apply configuration if explicitly requested. */
> > >         if (StreamKeyValueParser::updateConfiguration(config.get(),
> > >
> > > options_[OptStream])) {
> > > -               std::cerr << "Failed to update configuration" << std::endl;
> > > +               EPR("Failed to update configuration\n");
> > >                 return;
> > >         }
> > >
> > > @@ -72,20 +72,17 @@ CameraSession::CameraSession(CameraManager *cm,
> > >  #ifdef HAVE_KMS
> > >         if (options_.isSet(OptDisplay)) {
> > >                 if (options_.isSet(OptFile)) {
> > > -                       std::cerr << "--display and --file options are
> > > mutually exclusive"
> > > -                                 << std::endl;
> > > +                       EPR("--display and --file options are mutually
> > > exclusive\n");
> > >                         return;
> > >                 }
> > >
> > >                 if (roles.size() != 1) {
> > > -                       std::cerr << "Display doesn't support multiple
> > > streams"
> > > -                                 << std::endl;
> > > +                       EPR("Display doesn't support multiple streams\n");
> > >                         return;
> > >                 }
> > >
> > >                 if (roles[0] != StreamRole::Viewfinder) {
> > > -                       std::cerr << "Display requires a viewfinder stream"
> > > -                                 << std::endl;
> > > +                       EPR("Display requires a viewfinder stream\n");
> > >                         return;
> > >                 }
> > >         }
> > > @@ -97,15 +94,14 @@ CameraSession::CameraSession(CameraManager *cm,
> > >
> > >         case CameraConfiguration::Adjusted:
> > >                 if (strictFormats) {
> > > -                       std::cout << "Adjusting camera configuration
> > > disallowed by --strict-formats argument"
> > > -                                 << std::endl;
> > > +                       PR("Adjusting camera configuration disallowed by
> > > --strict-formats argument\n");
> > >                         return;
> > >                 }
> > > -               std::cout << "Camera configuration adjusted" << std::endl;
> > > +               PR("Camera configuration adjusted\n");
> > >                 break;
> > >
> > >         case CameraConfiguration::Invalid:
> > > -               std::cout << "Camera configuration invalid" << std::endl;
> > > +               PR("Camera configuration invalid\n");
> > >                 return;
> > >         }
> > >
> > > @@ -121,8 +117,7 @@ CameraSession::~CameraSession()
> > >  void CameraSession::listControls() const
> > >  {
> > >         for (const auto &[id, info] : camera_->controls()) {
> > > -               std::cout << "Control: " << id->name() << ": "
> > > -                         << info.toString() << std::endl;
> > > +               PR("Control: {}: {}}n", id->name(), info.toString());
> > >         }
> > >  }
> > >
> > > @@ -131,8 +126,7 @@ void CameraSession::listProperties() const
> > >         for (const auto &[key, value] : camera_->properties()) {
> > >                 const ControlId *id = properties::properties.at(key);
> > >
> > > -               std::cout << "Property: " << id->name() << " = "
> > > -                         << value.toString() << std::endl;
> > > +               PR("Property: {} = {}\n", id->name(), value.toString());
> > >         }
> > >  }
> > >
> > > @@ -140,17 +134,15 @@ void CameraSession::infoConfiguration() const
> > >  {
> > >         unsigned int index = 0;
> > >         for (const StreamConfiguration &cfg : *config_) {
> > > -               std::cout << index << ": " << cfg.toString() << std::endl;
> > > +               PR("{}: {}\n", index, cfg.toString());
> > >
> > >                 const StreamFormats &formats = cfg.formats();
> > >                 for (PixelFormat pixelformat : formats.pixelformats()) {
> > > -                       std::cout << " * Pixelformat: "
> > > -                                 << pixelformat << " "
> > > -                                 << formats.range(pixelformat).toString()
> > > -                                 << std::endl;
> > > +                       PR(" * Pixelformat: {} {}\n", pixelformat,
> > > +                          formats.range(pixelformat).toString());
> > >
> > >                         for (const Size &size : formats.sizes(pixelformat))
> > > -                               std::cout << "  - " << size << std::endl;
> > > +                               PR("  - {}\n", size);
> > >                 }
> > >
> > >                 index++;
> > > @@ -168,7 +160,7 @@ int CameraSession::start()
> > >
> > >         ret = camera_->configure(config_.get());
> > >         if (ret < 0) {
> > > -               std::cout << "Failed to configure camera" << std::endl;
> > > +               PR("Failed to configure camera\n");
> > >                 return ret;
> > >         }
> > >
> > > @@ -197,8 +189,7 @@ int CameraSession::start()
> > >         if (sink_) {
> > >                 ret = sink_->configure(*config_);
> > >                 if (ret < 0) {
> > > -                       std::cout << "Failed to configure frame sink"
> > > -                                 << std::endl;
> > > +                       PR("Failed to configure frame sink\n");
> > >                         return ret;
> > >                 }
> > >
> > > @@ -214,12 +205,12 @@ void CameraSession::stop()
> > >  {
> > >         int ret = camera_->stop();
> > >         if (ret)
> > > -               std::cout << "Failed to stop capture" << std::endl;
> > > +               PR("Failed to stop capture\n");
> > >
> > >         if (sink_) {
> > >                 ret = sink_->stop();
> > >                 if (ret)
> > > -                       std::cout << "Failed to stop frame sink" <<
> > > std::endl;
> > > +                       PR("Failed to stop frame sink\n");
> > >         }
> > >
> > >         sink_.reset();
> > > @@ -238,7 +229,7 @@ int CameraSession::startCapture()
> > >         for (StreamConfiguration &cfg : *config_) {
> > >                 ret = allocator_->allocate(cfg.stream());
> > >                 if (ret < 0) {
> > > -                       std::cerr << "Can't allocate buffers" << std::endl;
> > > +                       EPR("Can't allocate buffers\n");
> > >                         return -ENOMEM;
> > >                 }
> > >
> > > @@ -254,7 +245,7 @@ int CameraSession::startCapture()
> > >         for (unsigned int i = 0; i < nbuffers; i++) {
> > >                 std::unique_ptr<Request> request =
> > > camera_->createRequest();
> > >                 if (!request) {
> > > -                       std::cerr << "Can't create request" << std::endl;
> > > +                       EPR("Can't create request\n");
> > >                         return -ENOMEM;
> > >                 }
> > >
> > > @@ -266,8 +257,7 @@ int CameraSession::startCapture()
> > >
> > >                         ret = request->addBuffer(stream, buffer.get());
> > >                         if (ret < 0) {
> > > -                               std::cerr << "Can't set buffer for request"
> > > -                                         << std::endl;
> > > +                               EPR("Can't set buffer for request\n");
> > >                                 return ret;
> > >                         }
> > >
> > > @@ -281,14 +271,14 @@ int CameraSession::startCapture()
> > >         if (sink_) {
> > >                 ret = sink_->start();
> > >                 if (ret) {
> > > -                       std::cout << "Failed to start frame sink" <<
> > > std::endl;
> > > +                       PR("Failed to start frame sink\n");
> > >                         return ret;
> > >                 }
> > >         }
> > >
> > >         ret = camera_->start();
> > >         if (ret) {
> > > -               std::cout << "Failed to start capture" << std::endl;
> > > +               PR("Failed to start capture\n");
> > >                 if (sink_)
> > >                         sink_->stop();
> > >                 return ret;
> > > @@ -297,7 +287,7 @@ int CameraSession::startCapture()
> > >         for (std::unique_ptr<Request> &request : requests_) {
> > >                 ret = queueRequest(request.get());
> > >                 if (ret < 0) {
> > > -                       std::cerr << "Can't queue request" << std::endl;
> > > +                       EPR("Can't queue request\n");
> > >                         camera_->stop();
> > >                         if (sink_)
> > >                                 sink_->stop();
> > > @@ -306,13 +296,11 @@ int CameraSession::startCapture()
> > >         }
> > >
> > >         if (captureLimit_)
> > > -               std::cout << "cam" << cameraIndex_
> > > -                         << ": Capture " << captureLimit_ << " frames"
> > > -                         << std::endl;
> > > +               PR("cam{}: Capture {} frames\n", cameraIndex_,
> > > +                  captureLimit_);
> > >         else
> > > -               std::cout << "cam" << cameraIndex_
> > > -                         << ": Capture until user interrupts by SIGINT"
> > > -                         << std::endl;
> > > +               PR("cam{}: Capture until user interrupts by SIGINT\n",
> > > +                  cameraIndex_);
> > >
> > >         return 0;
> > >  }
> > > @@ -364,23 +352,23 @@ void CameraSession::processRequest(Request *request)
> > >
> > >         bool requeue = true;
> > >
> > > -       std::stringstream info;
> > > -       info << ts / 1000000000 << "."
> > > -            << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
> > > -            << " (" << std::fixed << std::setprecision(2) << fps << "
> > > fps)";
> > > +       auto sbuf = fmt::memory_buffer();
> > > +       fmt::format_to(std::back_inserter(sbuf), "{}.{:06} ({:.2f} fps)",
> > > +                      ts / 1000000000,
> > > +                      ts / 1000 % 1000000,
> > > +                      fps);
> > >
> > >         for (const auto &[stream, buffer] : buffers) {
> > >                 const FrameMetadata &metadata = buffer->metadata();
> > >
> > > -               info << " " << streamNames_[stream]
> > > -                    << " seq: " << std::setw(6) << std::setfill('0') <<
> > > metadata.sequence
> > > -                    << " bytesused: ";
> > > +               fmt::format_to(std::back_inserter(sbuf), " {} seq: {:06}
> > > bytesused: ",
> > > +                              streamNames_[stream], metadata.sequence);
> > >
> > >                 unsigned int nplane = 0;
> > >                 for (const FrameMetadata::Plane &plane :
> > > metadata.planes()) {
> > > -                       info << plane.bytesused;
> > > +                       fmt::format_to(std::back_inserter(sbuf), "{}",
> > > plane.bytesused);
> > >                         if (++nplane < metadata.planes().size())
> > > -                               info << "/";
> > > +                               fmt::format_to(std::back_inserter(sbuf),
> > > "/");
> > >                 }
> > >         }
> > >
> > > @@ -389,14 +377,13 @@ void CameraSession::processRequest(Request *request)
> > >                         requeue = false;
> > >         }
> > >
> > > -       std::cout << info.str() << std::endl;
> > > +       PR("{}\n", fmt::to_string(sbuf));
> > >
> > >         if (printMetadata_) {
> > >                 const ControlList &requestMetadata = request->metadata();
> > >                 for (const auto &[key, value] : requestMetadata) {
> > >                         const ControlId *id = controls::controls.at(key);
> > > -                       std::cout << "\t" << id->name() << " = "
> > > -                                 << value.toString() << std::endl;
> > > +                       PR("\t{} = {}\n", id->name(), value.toString());
> > >                 }
> > >         }
> > >
> > > diff --git a/src/cam/drm.cpp b/src/cam/drm.cpp
> > > index 46e34eb5..84919ab3 100644
> > > --- a/src/cam/drm.cpp
> > > +++ b/src/cam/drm.cpp
> > > @@ -10,12 +10,12 @@
> > >  #include <algorithm>
> > >  #include <errno.h>
> > >  #include <fcntl.h>
> > > -#include <iostream>
> > >  #include <set>
> > >  #include <string.h>
> > >  #include <sys/ioctl.h>
> > >  #include <sys/stat.h>
> > >  #include <sys/types.h>
> > > +#include <fmt/core.h>
> > >
> > >  #include <libcamera/framebuffer.h>
> > >  #include <libcamera/geometry.h>
> > > @@ -25,6 +25,9 @@
> > >
> > >  #include "event_loop.h"
> > >
> > > +#define PR(...) fmt::print(__VA_ARGS__)
> > > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> > > +
> > >  namespace DRM {
> > >
> > >  Object::Object(Device *dev, uint32_t id, Type type)
> > > @@ -178,9 +181,7 @@ Connector::Connector(Device *dev, const
> > > drmModeConnector *connector)
> > >  {
> > >         auto typeName = connectorTypeNames.find(connector->connector_type);
> > >         if (typeName == connectorTypeNames.end()) {
> > > -               std::cerr
> > > -                       << "Invalid connector type "
> > > -                       << connector->connector_type << std::endl;
> > > +               EPR("Invalid connector type {}}n",
> > > connector->connector_type);
> > >                 typeName =
> > > connectorTypeNames.find(DRM_MODE_CONNECTOR_Unknown);
> > >         }
> > >
> > > @@ -213,9 +214,7 @@ Connector::Connector(Device *dev, const
> > > drmModeConnector *connector)
> > >                                                     return e.id() ==
> > > encoderId;
> > >                                             });
> > >                 if (encoder == encoders.end()) {
> > > -                       std::cerr
> > > -                               << "Encoder " << encoderId << " not found"
> > > -                               << std::endl;
> > > +                       EPR("Encoder {} not found\n", encoderId);
> > >                         continue;
> > >                 }
> > >
> > > @@ -296,9 +295,7 @@ FrameBuffer::~FrameBuffer()
> > >
> > >                 if (ret == -1) {
> > >                         ret = -errno;
> > > -                       std::cerr
> > > -                               << "Failed to close GEM object: "
> > > -                               << strerror(-ret) << std::endl;
> > > +                       EPR("Failed to close GEM object: {}\n",
> > > strerror(-ret));
> > >                 }
> > >         }
> > >
> > > @@ -408,9 +405,8 @@ int Device::init()
> > >         fd_ = open(name, O_RDWR | O_CLOEXEC);
> > >         if (fd_ < 0) {
> > >                 ret = -errno;
> > > -               std::cerr
> > > -                       << "Failed to open DRM/KMS device " << name << ": "
> > > -                       << strerror(-ret) << std::endl;
> > > +               EPR("Failed to open DRM/KMS device {}: {}\n", name,
> > > +                   strerror(-ret));
> > >                 return ret;
> > >         }
> > >
> > > @@ -421,9 +417,7 @@ int Device::init()
> > >         ret = drmSetClientCap(fd_, DRM_CLIENT_CAP_ATOMIC, 1);
> > >         if (ret < 0) {
> > >                 ret = -errno;
> > > -               std::cerr
> > > -                       << "Failed to enable atomic capability: "
> > > -                       << strerror(-ret) << std::endl;
> > > +               EPR("Failed to enable atomic capability: {}\n",
> > > strerror(-ret));
> > >                 return ret;
> > >         }
> > >
> > > @@ -448,9 +442,7 @@ int Device::getResources()
> > >         };
> > >         if (!resources) {
> > >                 ret = -errno;
> > > -               std::cerr
> > > -                       << "Failed to get DRM/KMS resources: "
> > > -                       << strerror(-ret) << std::endl;
> > > +               EPR("Failed to get DRM/KMS resources: {}\n",
> > > strerror(-ret));
> > >                 return ret;
> > >         }
> > >
> > > @@ -458,9 +450,7 @@ int Device::getResources()
> > >                 drmModeCrtc *crtc = drmModeGetCrtc(fd_,
> > > resources->crtcs[i]);
> > >                 if (!crtc) {
> > >                         ret = -errno;
> > > -                       std::cerr
> > > -                               << "Failed to get CRTC: " << strerror(-ret)
> > > -                               << std::endl;
> > > +                       EPR("Failed to get CRTC: {}\n", strerror(-ret));
> > >                         return ret;
> > >                 }
> > >
> > > @@ -476,9 +466,7 @@ int Device::getResources()
> > >                         drmModeGetEncoder(fd_, resources->encoders[i]);
> > >                 if (!encoder) {
> > >                         ret = -errno;
> > > -                       std::cerr
> > > -                               << "Failed to get encoder: " <<
> > > strerror(-ret)
> > > -                               << std::endl;
> > > +                       EPR("Failed to get encoder: {}\n", strerror(-ret));
> > >                         return ret;
> > >                 }
> > >
> > > @@ -494,9 +482,7 @@ int Device::getResources()
> > >                         drmModeGetConnector(fd_, resources->connectors[i]);
> > >                 if (!connector) {
> > >                         ret = -errno;
> > > -                       std::cerr
> > > -                               << "Failed to get connector: " <<
> > > strerror(-ret)
> > > -                               << std::endl;
> > > +                       EPR("Failed to get connector: {}\n",
> > > strerror(-ret));
> > >                         return ret;
> > >                 }
> > >
> > > @@ -513,9 +499,7 @@ int Device::getResources()
> > >         };
> > >         if (!planes) {
> > >                 ret = -errno;
> > > -               std::cerr
> > > -                       << "Failed to get DRM/KMS planes: "
> > > -                       << strerror(-ret) << std::endl;
> > > +               EPR("Failed to get DRM/KMS planes: {}\n", strerror(-ret));
> > >                 return ret;
> > >         }
> > >
> > > @@ -524,9 +508,7 @@ int Device::getResources()
> > >                         drmModeGetPlane(fd_, planes->planes[i]);
> > >                 if (!plane) {
> > >                         ret = -errno;
> > > -                       std::cerr
> > > -                               << "Failed to get plane: " <<
> > > strerror(-ret)
> > > -                               << std::endl;
> > > +                       EPR("Failed to get plane: {}\n", strerror(-ret));
> > >                         return ret;
> > >                 }
> > >
> > > @@ -556,9 +538,7 @@ int Device::getResources()
> > >                 drmModePropertyRes *property = drmModeGetProperty(fd_, id);
> > >                 if (!property) {
> > >                         ret = -errno;
> > > -                       std::cerr
> > > -                               << "Failed to get property: " <<
> > > strerror(-ret)
> > > -                               << std::endl;
> > > +                       EPR("Failed to get property: {}\n",
> > > strerror(-ret));
> > >                         continue;
> > >                 }
> > >
> > > @@ -573,9 +553,8 @@ int Device::getResources()
> > >         for (auto &object : objects_) {
> > >                 ret = object.second->setup();
> > >                 if (ret < 0) {
> > > -                       std::cerr
> > > -                               << "Failed to setup object " <<
> > > object.second->id()
> > > -                               << ": " << strerror(-ret) << std::endl;
> > > +                       EPR("Failed to setup object {}: {}\n",
> > > +                           object.second->id(), strerror(-ret));
> > >                         return ret;
> > >                 }
> > >         }
> > > @@ -616,9 +595,8 @@ std::unique_ptr<FrameBuffer> Device::createFrameBuffer(
> > >                         ret = drmPrimeFDToHandle(fd_, plane.fd.get(),
> > > &handle);
> > >                         if (ret < 0) {
> > >                                 ret = -errno;
> > > -                               std::cerr
> > > -                                       << "Unable to import framebuffer
> > > dmabuf: "
> > > -                                       << strerror(-ret) << std::endl;
> > > +                               EPR("Unable to import framebuffer dmabuf:
> > > {}\n",
> > > +                                   strerror(-ret));
> > >                                 return nullptr;
> > >                         }
> > >
> > > @@ -636,9 +614,7 @@ std::unique_ptr<FrameBuffer> Device::createFrameBuffer(
> > >                             strides.data(), offsets, &fb->id_, 0);
> > >         if (ret < 0) {
> > >                 ret = -errno;
> > > -               std::cerr
> > > -                       << "Failed to add framebuffer: "
> > > -                       << strerror(-ret) << std::endl;
> > > +               EPR("Failed to add framebuffer: {}\n", strerror(-ret));
> > >                 return nullptr;
> > >         }
> > >
> > > diff --git a/src/cam/event_loop.cpp b/src/cam/event_loop.cpp
> > > index e25784c0..87aaf59a 100644
> > > --- a/src/cam/event_loop.cpp
> > > +++ b/src/cam/event_loop.cpp
> > > @@ -10,7 +10,10 @@
> > >  #include <assert.h>
> > >  #include <event2/event.h>
> > >  #include <event2/thread.h>
> > > -#include <iostream>
> > > +#include <fmt/core.h>
> > > +
> > > +#define PR(...) fmt::print(__VA_ARGS__)
> > > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> > >
> > >  EventLoop *EventLoop::instance_ = nullptr;
> > >
> > > @@ -71,13 +74,13 @@ void EventLoop::addEvent(int fd, EventType type,
> > >         event->event_ = event_new(base_, fd, events,
> > > &EventLoop::Event::dispatch,
> > >                                   event.get());
> > >         if (!event->event_) {
> > > -               std::cerr << "Failed to create event for fd " << fd <<
> > > std::endl;
> > > +               EPR("Failed to create event for fd {}\n", fd);
> > >                 return;
> > >         }
> > >
> > >         int ret = event_add(event->event_, nullptr);
> > >         if (ret < 0) {
> > > -               std::cerr << "Failed to add event for fd " << fd <<
> > > std::endl;
> > > +               EPR("Failed to add event for fd {}\n", fd);
> > >                 return;
> > >         }
> > >
> > > diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp
> > > index 45213d4a..86e2118c 100644
> > > --- a/src/cam/file_sink.cpp
> > > +++ b/src/cam/file_sink.cpp
> > > @@ -7,11 +7,12 @@
> > >
> > >  #include <assert.h>
> > >  #include <fcntl.h>
> > > -#include <iomanip>
> > > -#include <iostream>
> > > -#include <sstream>
> > >  #include <string.h>
> > >  #include <unistd.h>
> > > +#include <fmt/core.h>
> > > +
> > > +#define PR(...) fmt::print(__VA_ARGS__)
> > > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> > >
> > >  #include <libcamera/camera.h>
> > >
> > > @@ -70,10 +71,10 @@ void FileSink::writeBuffer(const Stream *stream,
> > > FrameBuffer *buffer)
> > >
> > >         pos = filename.find_first_of('#');
> > >         if (pos != std::string::npos) {
> > > -               std::stringstream ss;
> > > -               ss << streamNames_[stream] << "-" << std::setw(6)
> > > -                  << std::setfill('0') << buffer->metadata().sequence;
> > > -               filename.replace(pos, 1, ss.str());
> > > +               auto s = fmt::format("{}-{:06}",
> > > +                                    streamNames_[stream],
> > > +                                    buffer->metadata().sequence);
> > > +               filename.replace(pos, 1, s);
> > >         }
> > >
> > >         fd = open(filename.c_str(), O_CREAT | O_WRONLY |
> > > @@ -81,8 +82,7 @@ void FileSink::writeBuffer(const Stream *stream,
> > > FrameBuffer *buffer)
> > >                   S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH |
> > > S_IWOTH);
> > >         if (fd == -1) {
> > >                 ret = -errno;
> > > -               std::cerr << "failed to open file " << filename << ": "
> > > -                         << strerror(-ret) << std::endl;
> > > +               EPR("failed to open file {}: {}\n", filename,
> > > strerror(-ret));
> > >                 return;
> > >         }
> > >
> > > @@ -95,20 +95,17 @@ void FileSink::writeBuffer(const Stream *stream,
> > > FrameBuffer *buffer)
> > >                 unsigned int length = std::min<unsigned
> > > int>(meta.bytesused, data.size());
> > >
> > >                 if (meta.bytesused > data.size())
> > > -                       std::cerr << "payload size " << meta.bytesused
> > > -                                 << " larger than plane size " <<
> > > data.size()
> > > -                                 << std::endl;
> > > +                       EPR("payload size {} larger than plane size {}\n",
> > > +                           meta.bytesused, data.size());
> > >
> > >                 ret = ::write(fd, data.data(), length);
> > >                 if (ret < 0) {
> > >                         ret = -errno;
> > > -                       std::cerr << "write error: " << strerror(-ret)
> > > -                                 << std::endl;
> > > +                       EPR("write error: {}\n", strerror(-ret));
> > >                         break;
> > >                 } else if (ret != (int)length) {
> > > -                       std::cerr << "write error: only " << ret
> > > -                                 << " bytes written instead of "
> > > -                                 << length << std::endl;
> > > +                       EPR("write error: only {} bytes written instead of
> > > {}\n",
> > > +                           ret, length);
> > >                         break;
> > >                 }
> > >         }
> > > diff --git a/src/cam/image.cpp b/src/cam/image.cpp
> > > index fe2cc6da..73bcf915 100644
> > > --- a/src/cam/image.cpp
> > > +++ b/src/cam/image.cpp
> > > @@ -9,11 +9,14 @@
> > >
> > >  #include <assert.h>
> > >  #include <errno.h>
> > > -#include <iostream>
> > >  #include <map>
> > >  #include <string.h>
> > >  #include <sys/mman.h>
> > >  #include <unistd.h>
> > > +#include <fmt/core.h>
> > > +
> > > +#define PR(...) fmt::print(__VA_ARGS__)
> > > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> > >
> > >  using namespace libcamera;
> > >
> > > @@ -49,10 +52,8 @@ std::unique_ptr<Image> Image::fromFrameBuffer(const
> > > FrameBuffer *buffer, MapMode
> > >
> > >                 if (plane.offset > length ||
> > >                     plane.offset + plane.length > length) {
> > > -                       std::cerr << "plane is out of buffer: buffer
> > > length="
> > > -                                 << length << ", plane offset=" <<
> > > plane.offset
> > > -                                 << ", plane length=" << plane.length
> > > -                                 << std::endl;
> > > +                       EPR("plane is out of buffer: buffer length={},
> > > plane offset={}, plane length={}\n",
> > > +                           length, plane.offset, plane.length);
> > >                         return nullptr;
> > >                 }
> > >                 size_t &mapLength = mappedBuffers[fd].mapLength;
> > > @@ -68,8 +69,8 @@ std::unique_ptr<Image> Image::fromFrameBuffer(const
> > > FrameBuffer *buffer, MapMode
> > >                                              MAP_SHARED, fd, 0);
> > >                         if (address == MAP_FAILED) {
> > >                                 int error = -errno;
> > > -                               std::cerr << "Failed to mmap plane: "
> > > -                                         << strerror(-error) << std::endl;
> > > +                               EPR("Failed to mmap plane: {}\n",
> > > +                                   strerror(-error));
> > >                                 return nullptr;
> > >                         }
> > >
> > > diff --git a/src/cam/kms_sink.cpp b/src/cam/kms_sink.cpp
> > > index 7add81a6..823b75e4 100644
> > > --- a/src/cam/kms_sink.cpp
> > > +++ b/src/cam/kms_sink.cpp
> > > @@ -10,10 +10,13 @@
> > >  #include <array>
> > >  #include <algorithm>
> > >  #include <assert.h>
> > > -#include <iostream>
> > >  #include <memory>
> > >  #include <stdint.h>
> > >  #include <string.h>
> > > +#include <fmt/core.h>
> > > +
> > > +#define PR(...) fmt::print(__VA_ARGS__)
> > > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> > >
> > >  #include <libcamera/camera.h>
> > >  #include <libcamera/formats.h>
> > > @@ -54,11 +57,9 @@ KMSSink::KMSSink(const std::string &connectorName)
> > >
> > >         if (!connector_) {
> > >                 if (!connectorName.empty())
> > > -                       std::cerr
> > > -                               << "Connector " << connectorName << " not
> > > found"
> > > -                               << std::endl;
> > > +                       EPR("Connector {} not found\n", connectorName);
> > >                 else
> > > -                       std::cerr << "No connected connector found" <<
> > > std::endl;
> > > +                       EPR("No connected connector found\n");
> > >                 return;
> > >         }
> > >
> > > @@ -119,7 +120,7 @@ int KMSSink::configure(const
> > > libcamera::CameraConfiguration &config)
> > >                                                       mode.vdisplay ==
> > > cfg.size.height;
> > >                                        });
> > >         if (iter == modes.end()) {
> > > -               std::cerr << "No mode matching " << cfg.size << std::endl;
> > > +               EPR("No mode matching {}\n", cfg.size);
> > >                 return -EINVAL;
> > >         }
> > >
> > > @@ -192,17 +193,12 @@ int KMSSink::configurePipeline(const
> > > libcamera::PixelFormat &format)
> > >  {
> > >         const int ret = selectPipeline(format);
> > >         if (ret) {
> > > -               std::cerr
> > > -                       << "Unable to find display pipeline for format "
> > > -                       << format << std::endl;
> > > -
> > > +               EPR("Unable to find display pipeline for format {}\n",
> > > format);
> > >                 return ret;
> > >         }
> > >
> > > -       std::cout
> > > -               << "Using KMS plane " << plane_->id() << ", CRTC " <<
> > > crtc_->id()
> > > -               << ", connector " << connector_->name()
> > > -               << " (" << connector_->id() << ")" << std::endl;
> > > +       PR("Using KMS plane {}, CRTC {}, connector {} ({})\n",
> > > +          plane_->id(), crtc_->id(), connector_->name(),
> > > connector_->id());
> > >
> > >         return 0;
> > >  }
> > > @@ -228,9 +224,8 @@ int KMSSink::start()
> > >
> > >         ret = request->commit(DRM::AtomicRequest::FlagAllowModeset);
> > >         if (ret < 0) {
> > > -               std::cerr
> > > -                       << "Failed to disable CRTCs and planes: "
> > > -                       << strerror(-ret) << std::endl;
> > > +               EPR("Failed to disable CRTCs and planes: {}\n",
> > > +                   strerror(-ret));
> > >                 return ret;
> > >         }
> > >
> > > @@ -250,9 +245,7 @@ int KMSSink::stop()
> > >
> > >         int ret = request.commit(DRM::AtomicRequest::FlagAllowModeset);
> > >         if (ret < 0) {
> > > -               std::cerr
> > > -                       << "Failed to stop display pipeline: "
> > > -                       << strerror(-ret) << std::endl;
> > > +               EPR("Failed to stop display pipeline: {}\n",
> > > strerror(-ret));
> > >                 return ret;
> > >         }
> > >
> > > @@ -312,9 +305,8 @@ bool KMSSink::processRequest(libcamera::Request
> > > *camRequest)
> > >         if (!queued_) {
> > >                 int ret = drmRequest->commit(flags);
> > >                 if (ret < 0) {
> > > -                       std::cerr
> > > -                               << "Failed to commit atomic request: "
> > > -                               << strerror(-ret) << std::endl;
> > > +                       EPR("Failed to commit atomic request: {}\n",
> > > +                           strerror(-ret));
> > >                         /* \todo Implement error handling */
> > >                 }
> > >
> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > > index c7f664b9..03615dc9 100644
> > > --- a/src/cam/main.cpp
> > > +++ b/src/cam/main.cpp
> > > @@ -6,10 +6,9 @@
> > >   */
> > >
> > >  #include <atomic>
> > > -#include <iomanip>
> > > -#include <iostream>
> > >  #include <signal.h>
> > >  #include <string.h>
> > > +#include <fmt/core.h>
> > >
> > >  #include <libcamera/libcamera.h>
> > >  #include <libcamera/property_ids.h>
> > > @@ -78,8 +77,7 @@ int CamApp::init(int argc, char **argv)
> > >
> > >         ret = cm_->start();
> > >         if (ret) {
> > > -               std::cout << "Failed to start camera manager: "
> > > -                         << strerror(-ret) << std::endl;
> > > +               fmt::print("Failed to start camera manager: {}\n", -ret);
> > >                 return ret;
> > >         }
> > >
> > > @@ -173,12 +171,12 @@ int CamApp::parseOptions(int argc, char *argv[])
> > >
> > >  void CamApp::cameraAdded(std::shared_ptr<Camera> cam)
> > >  {
> > > -       std::cout << "Camera Added: " << cam->id() << std::endl;
> > > +       fmt::print("Camera Added: {}\n", cam->id());
> > >  }
> > >
> > >  void CamApp::cameraRemoved(std::shared_ptr<Camera> cam)
> > >  {
> > > -       std::cout << "Camera Removed: " << cam->id() << std::endl;
> > > +       fmt::print("Camera Removed: {}\n", cam->id());
> > >  }
> > >
> > >  void CamApp::captureDone()
> > > @@ -193,11 +191,11 @@ int CamApp::run()
> > >
> > >         /* 1. List all cameras. */
> > >         if (options_.isSet(OptList)) {
> > > -               std::cout << "Available cameras:" << std::endl;
> > > +               fmt::print("Available cameras:\n");
> > >
> > >                 unsigned int index = 1;
> > >                 for (const std::shared_ptr<Camera> &cam : cm_->cameras()) {
> > > -                       std::cout << index << ": " <<
> > > cameraName(cam.get()) << std::endl;
> > > +                       fmt::print("{}: {}\n", cameraName(cam.get()),
> > > index);
> > >                         index++;
> > >                 }
> > >         }
> > > @@ -215,12 +213,12 @@ int CamApp::run()
> > >                                                                 index,
> > >
> > > camera.children());
> > >                         if (!session->isValid()) {
> > > -                               std::cout << "Failed to create camera
> > > session" << std::endl;
> > > +                               fmt::print("Failed to create camera
> > > session\n");
> > >                                 return -EINVAL;
> > >                         }
> > >
> > > -                       std::cout << "Using camera " <<
> > > session->camera()->id()
> > > -                                 << " as cam" << index << std::endl;
> > > +                       fmt::print("Using camera{} as cam{}\n",
> > > +                                  session->camera()->id(), index);
> > >
> > >                         session->captureDone.connect(this,
> > > &CamApp::captureDone);
> > >
> > > @@ -250,7 +248,7 @@ int CamApp::run()
> > >
> > >                 ret = session->start();
> > >                 if (ret) {
> > > -                       std::cout << "Failed to start camera session" <<
> > > std::endl;
> > > +                       fmt::print("Failed to start camera session\n");
> > >                         return ret;
> > >                 }
> > >
> > > @@ -259,8 +257,8 @@ int CamApp::run()
> > >
> > >         /* 5. Enable hotplug monitoring. */
> > >         if (options_.isSet(OptMonitor)) {
> > > -               std::cout << "Monitoring new hotplug and unplug events" <<
> > > std::endl;
> > > -               std::cout << "Press Ctrl-C to interrupt" << std::endl;
> > > +               fmt::print("Monitoring new hotplug and unplug events\n");
> > > +               fmt::print("Press Ctrl-C to interrupt\n");
> > >
> > >                 cm_->cameraAdded.connect(this, &CamApp::cameraAdded);
> > >                 cm_->cameraRemoved.connect(this, &CamApp::cameraRemoved);
> > > @@ -323,7 +321,7 @@ std::string CamApp::cameraName(const Camera *camera)
> > >
> > >  void signalHandler([[maybe_unused]] int signal)
> > >  {
> > > -       std::cout << "Exiting" << std::endl;
> > > +       fmt::print("Exiting");
> > >         CamApp::instance()->quit();
> > >  }
> > >
> > > diff --git a/src/cam/meson.build b/src/cam/meson.build
> > > index 5bab8c9e..2b47383d 100644
> > > --- a/src/cam/meson.build
> > > +++ b/src/cam/meson.build
> > > @@ -7,6 +7,8 @@ if not libevent.found()
> > >      subdir_done()
> > >  endif
> > >
> > > +libfmt_dep = dependency('fmt')
> > > +
> > >  cam_enabled = true
> > >
> > >  cam_sources = files([
> > > @@ -25,7 +27,7 @@ cam_cpp_args = []
> > >  libdrm = dependency('libdrm', required : false)
> > >
> > >  if libdrm.found()
> > > -    cam_cpp_args += [ '-DHAVE_KMS' ]
> > > +    cam_cpp_args += [ '-DHAVE_KMS', ]
> > >      cam_sources += files([
> > >          'drm.cpp',
> > >          'kms_sink.cpp'
> > > @@ -38,6 +40,7 @@ cam  = executable('cam', cam_sources,
> > >                        libcamera_public,
> > >                        libdrm,
> > >                        libevent,
> > > +                      libfmt_dep,
> > >                    ],
> > >                    cpp_args : cam_cpp_args,
> > >                    install : true)
> > > diff --git a/src/cam/options.cpp b/src/cam/options.cpp
> > > index 4f7e8691..c9979385 100644
> > > --- a/src/cam/options.cpp
> > > +++ b/src/cam/options.cpp
> > > @@ -7,9 +7,11 @@
> > >
> > >  #include <assert.h>
> > >  #include <getopt.h>
> > > -#include <iomanip>
> > > -#include <iostream>
> > >  #include <string.h>
> > > +#include <fmt/core.h>
> > > +
> > > +#define PR(...) fmt::print(__VA_ARGS__)
> > > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> > >
> > >  #include "options.h"
> > >
> > > @@ -390,26 +392,23 @@ KeyValueParser::Options KeyValueParser::parse(const
> > > char *arguments)
> > >                         continue;
> > >
> > >                 if (optionsMap_.find(key) == optionsMap_.end()) {
> > > -                       std::cerr << "Invalid option " << key << std::endl;
> > > +                       EPR("Invalid option {}\n", key);
> > >                         return options;
> > >                 }
> > >
> > >                 OptionArgument arg = optionsMap_[key].argument;
> > >                 if (value.empty() && arg == ArgumentRequired) {
> > > -                       std::cerr << "Option " << key << " requires an
> > > argument"
> > > -                                 << std::endl;
> > > +                       EPR("Option {} requires an argument\n", key);
> > >                         return options;
> > >                 } else if (!value.empty() && arg == ArgumentNone) {
> > > -                       std::cerr << "Option " << key << " takes no
> > > argument"
> > > -                                 << std::endl;
> > > +                       EPR("Option {} takes no argument\n", key);
> > >                         return options;
> > >                 }
> > >
> > >                 const Option &option = optionsMap_[key];
> > >                 if (!options.parseValue(key, option, value.c_str())) {
> > > -                       std::cerr << "Failed to parse '" << value << "' as
> > > "
> > > -                                 << option.typeName() << " for option "
> > > << key
> > > -                                 << std::endl;
> > > +                       EPR("Failed to parse '{}' as {} for option {}\n",
> > > +                           value, option.typeName(), key);
> > >                         return options;
> > >                 }
> > >         }
> > > @@ -453,16 +452,16 @@ void KeyValueParser::usage(int indent)
> > >                                 argument += "]";
> > >                 }
> > >
> > > -               std::cerr << std::setw(indent) << argument;
> > > +               EPR("{:{}}", argument, indent);
> > >
> > >                 for (const char *help = option.help, *end = help; end;) {
> > >                         end = strchr(help, '\n');
> > >                         if (end) {
> > > -                               std::cerr << std::string(help, end - help
> > > + 1);
> > > -                               std::cerr << std::setw(indent) << " ";
> > > +                               EPR(std::string(help, end - help + 1));
> > > +                               EPR("{:{}}", "", indent);
> > >                                 help = end + 1;
> > >                         } else {
> > > -                               std::cerr << help << std::endl;
> > > +                               EPR("{}\n", help);
> > >                         }
> > >                 }
> > >         }
> > > @@ -929,10 +928,10 @@ OptionsParser::Options OptionsParser::parse(int
> > > argc, char **argv)
> > >
> > >                 if (c == '?' || c == ':') {
> > >                         if (c == '?')
> > > -                               std::cerr << "Invalid option ";
> > > +                               EPR("Invalid option ");
> > >                         else
> > > -                               std::cerr << "Missing argument for option
> > > ";
> > > -                       std::cerr << argv[optind - 1] << std::endl;
> > > +                               EPR("Missing argument for option ");
> > > +                       EPR("{}\n", argv[optind - 1]);
> > >
> > >                         usage();
> > >                         return options;
> > > @@ -946,8 +945,7 @@ OptionsParser::Options OptionsParser::parse(int argc,
> > > char **argv)
> > >         }
> > >
> > >         if (optind < argc) {
> > > -               std::cerr << "Invalid non-option argument '" <<
> > > argv[optind]
> > > -                         << "'" << std::endl;
> > > +               EPR("Invalid non-option argument '{}'\n", argv[optind]);
> > >                 usage();
> > >                 return options;
> > >         }
> > > @@ -992,14 +990,9 @@ void OptionsParser::usage()
> > >
> > >         indent = (indent + 7) / 8 * 8;
> > >
> > > -       std::cerr << "Options:" << std::endl;
> > > -
> > > -       std::ios_base::fmtflags f(std::cerr.flags());
> > > -       std::cerr << std::left;
> > > +       EPR("Options:\n");
> > >
> > >         usageOptions(options_, indent);
> > > -
> > > -       std::cerr.flags(f);
> > >  }
> > >
> > >  void OptionsParser::usageOptions(const std::list<Option> &options,
> > > @@ -1036,16 +1029,16 @@ void OptionsParser::usageOptions(const
> > > std::list<Option> &options,
> > >                 if (option.isArray)
> > >                         argument += " ...";
> > >
> > > -               std::cerr << std::setw(indent) << argument;
> > > +               EPR("{:{}}", argument, indent);
> > >
> > > -               for (const char *help = option.help, *end = help; end; ) {
> > > +               for (const char *help = option.help, *end = help; end;) {
> > >                         end = strchr(help, '\n');
> > >                         if (end) {
> > > -                               std::cerr << std::string(help, end - help
> > > + 1);
> > > -                               std::cerr << std::setw(indent) << " ";
> > > +                               EPR(std::string(help, end - help + 1));
> > > +                               EPR("{:{}}", "", indent);
> > >                                 help = end + 1;
> > >                         } else {
> > > -                               std::cerr << help << std::endl;
> > > +                               EPR("{}\n", help);
> > >                         }
> > >                 }
> > >
> > > @@ -1060,8 +1053,8 @@ void OptionsParser::usageOptions(const
> > > std::list<Option> &options,
> > >                 return;
> > >
> > >         for (const Option *option : parentOptions) {
> > > -               std::cerr << std::endl << "Options valid in the context of
> > > "
> > > -                         << option->optionName() << ":" << std::endl;
> > > +               EPR("\nOptions valid in the context of {}:\n",
> > > +                   option->optionName());
> > >                 usageOptions(option->children, indent);
> > >         }
> > >  }
> > > @@ -1125,15 +1118,14 @@ bool OptionsParser::parseValue(const Option
> > > &option, const char *arg,
> > >
> > >         std::tie(options, error) = childOption(option.parent, options);
> > >         if (error) {
> > > -               std::cerr << "Option " << option.optionName() << "
> > > requires a "
> > > -                         << error->optionName() << " context" <<
> > > std::endl;
> > > +               EPR("Option {} requires a {} context\n",
> > > +                   option.optionName(), error->optionName());
> > >                 return false;
> > >         }
> > >
> > >         if (!options->parseValue(option.opt, option, arg)) {
> > > -               std::cerr << "Can't parse " << option.typeName()
> > > -                         << " argument for option " << option.optionName()
> > > -                         << std::endl;
> > > +               EPR("Can't parse {} argument for option {}\n",
> > > +                   option.typeName(), option.optionName());
> > >                 return false;
> > >         }
> > >
> > > diff --git a/src/cam/stream_options.cpp b/src/cam/stream_options.cpp
> > > index 150bd27c..666862eb 100644
> > > --- a/src/cam/stream_options.cpp
> > > +++ b/src/cam/stream_options.cpp
> > > @@ -6,7 +6,10 @@
> > >   */
> > >  #include "stream_options.h"
> > >
> > > -#include <iostream>
> > > +#include <fmt/core.h>
> > > +
> > > +#define PR(...) fmt::print(__VA_ARGS__)
> > > +#define EPR(...) fmt::print(stderr, __VA_ARGS__)
> > >
> > >  using namespace libcamera;
> > >
> > > @@ -30,8 +33,7 @@ KeyValueParser::Options
> > > StreamKeyValueParser::parse(const char *arguments)
> > >
> > >         if (options.valid() && options.isSet("role") &&
> > >             !parseRole(&role, options)) {
> > > -               std::cerr << "Unknown stream role "
> > > -                         << options["role"].toString() << std::endl;
> > > +               EPR("Unknown stream role {}\n",
> > > options["role"].toString());
> > >                 options.invalidate();
> > >         }
> > >
> > > @@ -64,7 +66,7 @@ int
> > > StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
> > >                                               const OptionValue &values)
> > >  {
> > >         if (!config) {
> > > -               std::cerr << "No configuration provided" << std::endl;
> > > +               EPR("No configuration provided\n");
> > >                 return -EINVAL;
> > >         }
> > >
> > > @@ -75,12 +77,8 @@ int
> > > StreamKeyValueParser::updateConfiguration(CameraConfiguration *config,
> > >         const std::vector<OptionValue> &streamParameters =
> > > values.toArray();
> > >
> > >         if (config->size() != streamParameters.size()) {
> > > -               std::cerr
> > > -                       << "Number of streams in configuration "
> > > -                       << config->size()
> > > -                       << " does not match number of streams parsed "
> > > -                       << streamParameters.size()
> > > -                       << std::endl;
> > > +               EPR("Number of streams in configuration {} does not match
> > > number of streams parsed {}\n",
> > > +                   config->size(), streamParameters.size());
> > >                 return -EINVAL;
> > >         }
> > >
> > > --
> > > 2.34.1
> > >
> > >
>

Laurent Pinchart May 10, 2022, 1:04 p.m. UTC | #5

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@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).

> 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.

Laurent Pinchart May 10, 2022, 1:13 p.m. UTC | #6

Hi Tomi,

On Tue, May 10, 2022 at 12:10:21PM +0300, Tomi Valkeinen wrote:
> On 10/05/2022 10:16, Tomi Valkeinen 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.
> 
> As for the PR and EPR macros, those was just something to get forward 
> with. I think fmt::print("") is fine, but fmt::print(stderr, "") is a 
> bit long.
> 
> Compared to cout, "fmt::print(" is actually shorter than "std::cout << 
> ", and without all those << and std::endl the lines are shorter.
> 
> Not so with cerr and fmt::print(stderr, (although in many cases the 
> total length would still be shorter) but I think it makes sense to have 
> a macro/inline func for error prints, if only to make it more obvious 
> that it's an error print.
> 
> And, of course, libfmt could also be used for logging:
> 
> LOG(Camera, Error) << "Camera in " << camera_state_names[currentState]
> 		   << " state trying " << from << "() requiring state "
> 		   << camera_state_names[state];
> 
> to
> 
> LOG(Camera, Error, "Camera in {} state trying {}() requiring state {}",
>      camera_state_names[currentState], from, camera_state_names[state]);

That's an interesting example, it clearly shows one of the main semantic
differences between the two options. std::format makes it easier to see
the whole message as it's stored in a single string, but the drawback is
that you have to go back and forth between the format string and the
arguments to figure out what is getting printed. std::ostream makes the
reading flow more natural in my opinion.

I'm pretty sure my preference will vary based on the logging statement,
some will become more readable, others will get worse. That won't help
making a decision :-) It would be nice, however, to see how it would
affect logging, with a sample patch for the logger. It should ideally
also add a custom formatter for at least one of the geometry classes.

This being said, we don't have to switch to libfmt (assuming we want to
do so) everywhere, or everywhere at the same time. The cam application
and the unit tests could possibly go first, as they make heavy use of
std::cout and std::cerr (for the unit tests it would actually be nice to
have a better form of status reporting, std::cerr may be best replaced
with assertions that raise exceptions).

Laurent Pinchart May 10, 2022, 1:22 p.m. UTC | #7

Hi Tomi,

Thank you for the patch.

On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen 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've got it wrong. The first library you should add to any new C++
project you make is libcamera :-)

> 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.

Is this because libfmt is a header-only library ? What if it is built as
a shared object, what's the impact on code size in cam ?

> Converting that code to use fmt::format() and
> naive string append gives:
> 
> release with string append	233680
> release lto with string append	186936

What's the impact of switching to string concatenation on runtime ?

Would you consider the option of keeping std::stringstream ?

> 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@ideasonboard.com>
> ---
>  src/cam/camera_session.cpp | 105 ++++++++++++++++---------------------
>  src/cam/drm.cpp            |  68 ++++++++----------------
>  src/cam/event_loop.cpp     |   9 ++--
>  src/cam/file_sink.cpp      |  31 +++++------
>  src/cam/image.cpp          |  15 +++---
>  src/cam/kms_sink.cpp       |  38 ++++++--------
>  src/cam/main.cpp           |  28 +++++-----
>  src/cam/meson.build        |   5 +-
>  src/cam/options.cpp        |  66 ++++++++++-------------
>  src/cam/stream_options.cpp |  18 +++----
>  10 files changed, 165 insertions(+), 218 deletions(-)

[snip]

Laurent Pinchart May 10, 2022, 1:24 p.m. UTC | #8

On Tue, May 10, 2022 at 04:22:51PM +0300, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen 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've got it wrong. The first library you should add to any new C++
> project you make is libcamera :-)
> 
> > 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.
> 
> Is this because libfmt is a header-only library ? What if it is built as
> a shared object, what's the impact on code size in cam ?

It would also be useful to make the same measurements using the C++20
std::format, as that would be the long term goal.

> > Converting that code to use fmt::format() and
> > naive string append gives:
> > 
> > release with string append	233680
> > release lto with string append	186936
> 
> What's the impact of switching to string concatenation on runtime ?
> 
> Would you consider the option of keeping std::stringstream ?
> 
> > 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@ideasonboard.com>
> > ---
> >  src/cam/camera_session.cpp | 105 ++++++++++++++++---------------------
> >  src/cam/drm.cpp            |  68 ++++++++----------------
> >  src/cam/event_loop.cpp     |   9 ++--
> >  src/cam/file_sink.cpp      |  31 +++++------
> >  src/cam/image.cpp          |  15 +++---
> >  src/cam/kms_sink.cpp       |  38 ++++++--------
> >  src/cam/main.cpp           |  28 +++++-----
> >  src/cam/meson.build        |   5 +-
> >  src/cam/options.cpp        |  66 ++++++++++-------------
> >  src/cam/stream_options.cpp |  18 +++----
> >  10 files changed, 165 insertions(+), 218 deletions(-)
> 
> [snip]

Tomi Valkeinen May 10, 2022, 1:47 p.m. UTC | #9

On 10/05/2022 16:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tue, May 10, 2022 at 12:10:21PM +0300, Tomi Valkeinen wrote:
>> On 10/05/2022 10:16, Tomi Valkeinen 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.
>>
>> As for the PR and EPR macros, those was just something to get forward
>> with. I think fmt::print("") is fine, but fmt::print(stderr, "") is a
>> bit long.
>>
>> Compared to cout, "fmt::print(" is actually shorter than "std::cout <<
>> ", and without all those << and std::endl the lines are shorter.
>>
>> Not so with cerr and fmt::print(stderr, (although in many cases the
>> total length would still be shorter) but I think it makes sense to have
>> a macro/inline func for error prints, if only to make it more obvious
>> that it's an error print.
>>
>> And, of course, libfmt could also be used for logging:
>>
>> LOG(Camera, Error) << "Camera in " << camera_state_names[currentState]
>> 		   << " state trying " << from << "() requiring state "
>> 		   << camera_state_names[state];
>>
>> to
>>
>> LOG(Camera, Error, "Camera in {} state trying {}() requiring state {}",
>>       camera_state_names[currentState], from, camera_state_names[state]);
> 
> That's an interesting example, it clearly shows one of the main semantic
> differences between the two options. std::format makes it easier to see
> the whole message as it's stored in a single string, but the drawback is
> that you have to go back and forth between the format string and the
> arguments to figure out what is getting printed. std::ostream makes the
> reading flow more natural in my opinion.

I think the bigger difference comes apparent when using more complex 
formats than just plain {} (i.e. setting precision, width, alignment, etc).

> I'm pretty sure my preference will vary based on the logging statement,
> some will become more readable, others will get worse. That won't help
> making a decision :-) It would be nice, however, to see how it would

Maybe there's a logging statement out there which is more readable with 
streams, but I'm not aware of it ;).

> affect logging, with a sample patch for the logger. It should ideally
> also add a custom formatter for at least one of the geometry classes.

We already have that in this patch, as camera_session.cpp prints a Size. 
But it uses the existing std::ostream formatter from geometry.cpp, so... 
it's a bit boring =).

libfmt has its own formatter support, which is useful if you want to 
pass formatting parameters to the formatter. Say, {:#x} and {:.2f} could 
be used to print "(0x4, 0x7)" and "(4.00, 7.00)" for the same Size.

  Tomi

Laurent Pinchart May 10, 2022, 1:54 p.m. UTC | #10

Hi Tomi,

On Tue, May 10, 2022 at 04:47:56PM +0300, Tomi Valkeinen wrote:
> On 10/05/2022 16:13, Laurent Pinchart wrote:
> > On Tue, May 10, 2022 at 12:10:21PM +0300, Tomi Valkeinen wrote:
> >> On 10/05/2022 10:16, Tomi Valkeinen 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.
> >>
> >> As for the PR and EPR macros, those was just something to get forward
> >> with. I think fmt::print("") is fine, but fmt::print(stderr, "") is a
> >> bit long.
> >>
> >> Compared to cout, "fmt::print(" is actually shorter than "std::cout <<
> >> ", and without all those << and std::endl the lines are shorter.
> >>
> >> Not so with cerr and fmt::print(stderr, (although in many cases the
> >> total length would still be shorter) but I think it makes sense to have
> >> a macro/inline func for error prints, if only to make it more obvious
> >> that it's an error print.
> >>
> >> And, of course, libfmt could also be used for logging:
> >>
> >> LOG(Camera, Error) << "Camera in " << camera_state_names[currentState]
> >> 		   << " state trying " << from << "() requiring state "
> >> 		   << camera_state_names[state];
> >>
> >> to
> >>
> >> LOG(Camera, Error, "Camera in {} state trying {}() requiring state {}",
> >>       camera_state_names[currentState], from, camera_state_names[state]);
> > 
> > That's an interesting example, it clearly shows one of the main semantic
> > differences between the two options. std::format makes it easier to see
> > the whole message as it's stored in a single string, but the drawback is
> > that you have to go back and forth between the format string and the
> > arguments to figure out what is getting printed. std::ostream makes the
> > reading flow more natural in my opinion.
> 
> I think the bigger difference comes apparent when using more complex 
> formats than just plain {} (i.e. setting precision, width, alignment, etc).

Once you get used to how the precision and other parameters are
expressed with libfmt, it's indeed more concise, possibly easier to
read, and certainly easier to write.

> > I'm pretty sure my preference will vary based on the logging statement,
> > some will become more readable, others will get worse. That won't help
> > making a decision :-) It would be nice, however, to see how it would
> 
> Maybe there's a logging statement out there which is more readable with 
> streams, but I'm not aware of it ;).

I think the above example is more readable with streams :-)

> > affect logging, with a sample patch for the logger. It should ideally
> > also add a custom formatter for at least one of the geometry classes.
> 
> We already have that in this patch, as camera_session.cpp prints a Size. 
> But it uses the existing std::ostream formatter from geometry.cpp, so... 
> it's a bit boring =).
> 
> libfmt has its own formatter support, which is useful if you want to 
> pass formatting parameters to the formatter. Say, {:#x} and {:.2f} could 
> be used to print "(0x4, 0x7)" and "(4.00, 7.00)" for the same Size.

I meant using custom libfmt formatters, yes, the same way we have custom
operator<<() for the geometry classes. I'd like to see how that would
look like.

Tomi Valkeinen May 10, 2022, 2:05 p.m. UTC | #11

On 10/05/2022 16:22, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen 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've got it wrong. The first library you should add to any new C++
> project you make is libcamera :-)
> 
>> 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.
> 
> Is this because libfmt is a header-only library ? What if it is built as
> a shared object, what's the impact on code size in cam ?

libfmt supports header-only, but it's not header-only here.

>> Converting that code to use fmt::format() and
>> naive string append gives:
>>
>> release with string append	233680
>> release lto with string append	186936
> 
> What's the impact of switching to string concatenation on runtime ?

What do you mean?

> Would you consider the option of keeping std::stringstream ?

You mean formatting mostly with libfmt, but sometimes with streams? Or 
using libfmt but appending the formatted string to stringstream?

  Tomi

Laurent Pinchart May 10, 2022, 2:20 p.m. UTC | #12

Hi Tomi,

On Tue, May 10, 2022 at 05:05:05PM +0300, Tomi Valkeinen wrote:
> On 10/05/2022 16:22, Laurent Pinchart wrote:
> > On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen 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've got it wrong. The first library you should add to any new C++
> > project you make is libcamera :-)
> > 
> >> 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.
> > 
> > Is this because libfmt is a header-only library ? What if it is built as
> > a shared object, what's the impact on code size in cam ?
> 
> libfmt supports header-only, but it's not header-only here.
> 
> >> Converting that code to use fmt::format() and
> >> naive string append gives:
> >>
> >> release with string append	233680
> >> release lto with string append	186936
> > 
> > What's the impact of switching to string concatenation on runtime ?
> 
> What do you mean?

I mean what is the impact on CPU usage of using the "naive" string
append, compared to std::stringstream or fmt::memory_buffer ?

> > Would you consider the option of keeping std::stringstream ?
> 
> You mean formatting mostly with libfmt, but sometimes with streams? Or 
> using libfmt but appending the formatted string to stringstream?

Mostly using fmt::, but using std::stringstream without
fmt::memory_buffer when a string needs to be assembled. I'm not saying
it's necessarily a good idea, just wondering.

Eric Curtin May 21, 2022, 6:33 p.m. UTC | #13

On Tue, 10 May 2022 at 15:21, Laurent Pinchart via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi Tomi,
>
> On Tue, May 10, 2022 at 05:05:05PM +0300, Tomi Valkeinen wrote:
> > On 10/05/2022 16:22, Laurent Pinchart wrote:
> > > On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen 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've got it wrong. The first library you should add to any new C++
> > > project you make is libcamera :-)
> > >
> > >> 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.
> > >
> > > Is this because libfmt is a header-only library ? What if it is built as
> > > a shared object, what's the impact on code size in cam ?
> >
> > libfmt supports header-only, but it's not header-only here.
> >
> > >> Converting that code to use fmt::format() and
> > >> naive string append gives:
> > >>
> > >> release with string append 233680
> > >> release lto with string append     186936
> > >
> > > What's the impact of switching to string concatenation on runtime ?
> >
> > What do you mean?
>
> I mean what is the impact on CPU usage of using the "naive" string
> append, compared to std::stringstream or fmt::memory_buffer ?
>
> > > Would you consider the option of keeping std::stringstream ?
> >
> > You mean formatting mostly with libfmt, but sometimes with streams? Or
> > using libfmt but appending the formatted string to stringstream?
>
> Mostly using fmt::, but using std::stringstream without
> fmt::memory_buffer when a string needs to be assembled. I'm not saying
> it's necessarily a good idea, just wondering.
>
> --
> Regards,
>
> Laurent Pinchart
>

Something I wrote which you can use to format C++ strings in a printf-style

```
  #pragma once

  #include <string.h>
  #include <string>

  // function that will sprintf to a C++ string starting from
std::string::size()
  // so if you want to completely overwrite a string or start at a
specific point
  // use std::string::clear() or std::string::resize(). str is a std::string.
  #define STRING_PRINTF(str, ...)                                   \
    do {                                                            \
      const int size = snprintf(NULL, 0, __VA_ARGS__);              \
      const size_t start_of_string = str.size();                    \
      str.resize(start_of_string + size);                           \
      snprintf(&str[start_of_string], str.size() + 1, __VA_ARGS__); \
    } while (0)
```

This way you can format C++ strings in a sprintf-style. It would give
you many of the benefits of fmt with just a few lines of code
alternatively. I know we don't only use macros, but it makes sense
here because when the compiler see's the snprintf, it warns against
incorrect format strings when you do it this way.

Just an option...

Laurent Pinchart May 22, 2022, 9:58 a.m. UTC | #14

Hi Eric,

On Sat, May 21, 2022 at 07:33:03PM +0100, Eric Curtin wrote:
> On Tue, 10 May 2022 at 15:21, Laurent Pinchart wrote:
> > On Tue, May 10, 2022 at 05:05:05PM +0300, Tomi Valkeinen wrote:
> > > On 10/05/2022 16:22, Laurent Pinchart wrote:
> > > > On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen 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've got it wrong. The first library you should add to any new C++
> > > > project you make is libcamera :-)
> > > >
> > > >> 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.
> > > >
> > > > Is this because libfmt is a header-only library ? What if it is built as
> > > > a shared object, what's the impact on code size in cam ?
> > >
> > > libfmt supports header-only, but it's not header-only here.
> > >
> > > >> Converting that code to use fmt::format() and
> > > >> naive string append gives:
> > > >>
> > > >> release with string append 233680
> > > >> release lto with string append     186936
> > > >
> > > > What's the impact of switching to string concatenation on runtime ?
> > >
> > > What do you mean?
> >
> > I mean what is the impact on CPU usage of using the "naive" string
> > append, compared to std::stringstream or fmt::memory_buffer ?
> >
> > > > Would you consider the option of keeping std::stringstream ?
> > >
> > > You mean formatting mostly with libfmt, but sometimes with streams? Or
> > > using libfmt but appending the formatted string to stringstream?
> >
> > Mostly using fmt::, but using std::stringstream without
> > fmt::memory_buffer when a string needs to be assembled. I'm not saying
> > it's necessarily a good idea, just wondering.
> 
> Something I wrote which you can use to format C++ strings in a printf-style
> 
> ```
>   #pragma once
> 
>   #include <string.h>
>   #include <string>
> 
>   // function that will sprintf to a C++ string starting from std::string::size()
>   // so if you want to completely overwrite a string or start at a specific point
>   // use std::string::clear() or std::string::resize(). str is a std::string.
>   #define STRING_PRINTF(str, ...)                                   \
>     do {                                                            \
>       const int size = snprintf(NULL, 0, __VA_ARGS__);              \
>       const size_t start_of_string = str.size();                    \
>       str.resize(start_of_string + size);                           \
>       snprintf(&str[start_of_string], str.size() + 1, __VA_ARGS__); \
>     } while (0)
> ```
> 
> This way you can format C++ strings in a sprintf-style. It would give
> you many of the benefits of fmt with just a few lines of code
> alternatively. I know we don't only use macros, but it makes sense
> here because when the compiler see's the snprintf, it warns against
> incorrect format strings when you do it this way.

Beside the fact that snprintf() is called twice which may be less
efficient (although maybe still more efficient than libfmt), the trouble
here is that __VA_ARGS__ will be evaluated twice.

	unsigned int index;
	std::string str;

	index = 0;
	STRING_PRINTF(str, "%u", index++);
	/* Now index is equal to 2 */

> Just an option...

Kieran Bingham May 22, 2022, 10:08 a.m. UTC | #15

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@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

Laurent Pinchart May 22, 2022, 10:52 a.m. UTC | #16

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.

> 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.

Eric Curtin May 22, 2022, 8 p.m. UTC | #17

On Sun, 22 May 2022 at 10:58, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Eric,
>
> On Sat, May 21, 2022 at 07:33:03PM +0100, Eric Curtin wrote:
> > On Tue, 10 May 2022 at 15:21, Laurent Pinchart wrote:
> > > On Tue, May 10, 2022 at 05:05:05PM +0300, Tomi Valkeinen wrote:
> > > > On 10/05/2022 16:22, Laurent Pinchart wrote:
> > > > > On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen 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've got it wrong. The first library you should add to any new C++
> > > > > project you make is libcamera :-)
> > > > >
> > > > >> 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.
> > > > >
> > > > > Is this because libfmt is a header-only library ? What if it is built as
> > > > > a shared object, what's the impact on code size in cam ?
> > > >
> > > > libfmt supports header-only, but it's not header-only here.
> > > >
> > > > >> Converting that code to use fmt::format() and
> > > > >> naive string append gives:
> > > > >>
> > > > >> release with string append 233680
> > > > >> release lto with string append     186936
> > > > >
> > > > > What's the impact of switching to string concatenation on runtime ?
> > > >
> > > > What do you mean?
> > >
> > > I mean what is the impact on CPU usage of using the "naive" string
> > > append, compared to std::stringstream or fmt::memory_buffer ?
> > >
> > > > > Would you consider the option of keeping std::stringstream ?
> > > >
> > > > You mean formatting mostly with libfmt, but sometimes with streams? Or
> > > > using libfmt but appending the formatted string to stringstream?
> > >
> > > Mostly using fmt::, but using std::stringstream without
> > > fmt::memory_buffer when a string needs to be assembled. I'm not saying
> > > it's necessarily a good idea, just wondering.
> >
> > Something I wrote which you can use to format C++ strings in a printf-style
> >
> > ```
> >   #pragma once
> >
> >   #include <string.h>
> >   #include <string>
> >
> >   // function that will sprintf to a C++ string starting from std::string::size()
> >   // so if you want to completely overwrite a string or start at a specific point
> >   // use std::string::clear() or std::string::resize(). str is a std::string.
> >   #define STRING_PRINTF(str, ...)                                   \
> >     do {                                                            \
> >       const int size = snprintf(NULL, 0, __VA_ARGS__);              \
> >       const size_t start_of_string = str.size();                    \
> >       str.resize(start_of_string + size);                           \
> >       snprintf(&str[start_of_string], str.size() + 1, __VA_ARGS__); \
> >     } while (0)
> > ```
> >
> > This way you can format C++ strings in a sprintf-style. It would give
> > you many of the benefits of fmt with just a few lines of code
> > alternatively. I know we don't only use macros, but it makes sense
> > here because when the compiler see's the snprintf, it warns against
> > incorrect format strings when you do it this way.
>
> Beside the fact that snprintf() is called twice which may be less
> efficient (although maybe still more efficient than libfmt), the trouble
> here is that __VA_ARGS__ will be evaluated twice.
>
>         unsigned int index;
>         std::string str;
>
>         index = 0;
>         STRING_PRINTF(str, "%u", index++);

These simple cases could use:

  std::string str = std::to_string(index++);

It's unlikely that you are gonna beat std::to_string for a simple
conversion like this (well, libfmt beats everything in terms of
execution speed but it's another dependency, I've done a few
benchmarks in the past). STRING_PRINTF would easily convert to
std::format in future. The solution is efficient in terms of memory
but will do an evaluation twice as you said. The above macro was
written with readability and future compatibility to std::format in
mind, and is not very different to what folly from Meta does (only
much simpler and compiler friendly), see stringPrintf

https://github.com/facebook/folly/blob/main/folly/String.cpp

STRING_PRINTF suits the case where you have a longer format string and
you want to do something a bit more complex, so something like:

        std::stringstream info;
        info << ts / 1000000000 << "."
             << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
             << " (" << std::fixed << std::setprecision(2) << fps << " fps)";

        for (const auto &[stream, buffer] : buffers) {
                const FrameMetadata &metadata = buffer->metadata();

                info << " " << streamNames_[stream]
                     << " seq: " << std::setw(6) << std::setfill('0')
<< metadata.sequence
                     << " bytesused: ";

                unsigned int nplane = 0;
                for (const FrameMetadata::Plane &plane : metadata.planes()) {
                        info << plane.bytesused;
                        if (++nplane < metadata.planes().size())
                                info << "/";
                }
        }

        if (sink_) {
                if (!sink_->processRequest(request))
                        requeue = false;
        }

        std::cout << info.str() << std::endl;

would become:

        std::string info
        STRING_PRINTF(info, "%.6f (%.2f fps)", ts / 1000000000.0, fps);

        for (const auto &[stream, buffer] : buffers) {
                const FrameMetadata &metadata = buffer->metadata();

                STRING_PRINTF(info,
                             " %s seq: %06d bytesused: ",
streamNames_[buf.first].c_str(),
                             metadata.sequence);

                unsigned int nplane = 0;
                for (const FrameMetadata::Plane &plane : metadata.planes()) {
                        frame_str += std::to_string(plane.bytesused);
                        if (++nplane < metadata.planes().size())
                                info += "/";
                }
        }

        if (sink_) {
                if (!sink_->processRequest(request))
                        requeue = false;
        }

        printf("%s\n", info.c_str());

>         /* Now index is equal to 2 */
>
> > Just an option...
>
> --
> Regards,
>
> Laurent Pinchart
>

Laurent Pinchart May 23, 2022, 8:23 a.m. UTC | #18

Hi Eric,

On Sun, May 22, 2022 at 09:00:04PM +0100, Eric Curtin wrote:
> On Sun, 22 May 2022 at 10:58, Laurent Pinchart wrote:
> > On Sat, May 21, 2022 at 07:33:03PM +0100, Eric Curtin wrote:
> > > On Tue, 10 May 2022 at 15:21, Laurent Pinchart wrote:
> > > > On Tue, May 10, 2022 at 05:05:05PM +0300, Tomi Valkeinen wrote:
> > > > > On 10/05/2022 16:22, Laurent Pinchart wrote:
> > > > > > On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen 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've got it wrong. The first library you should add to any new C++
> > > > > > project you make is libcamera :-)
> > > > > >
> > > > > >> 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.
> > > > > >
> > > > > > Is this because libfmt is a header-only library ? What if it is built as
> > > > > > a shared object, what's the impact on code size in cam ?
> > > > >
> > > > > libfmt supports header-only, but it's not header-only here.
> > > > >
> > > > > >> Converting that code to use fmt::format() and
> > > > > >> naive string append gives:
> > > > > >>
> > > > > >> release with string append 233680
> > > > > >> release lto with string append     186936
> > > > > >
> > > > > > What's the impact of switching to string concatenation on runtime ?
> > > > >
> > > > > What do you mean?
> > > >
> > > > I mean what is the impact on CPU usage of using the "naive" string
> > > > append, compared to std::stringstream or fmt::memory_buffer ?
> > > >
> > > > > > Would you consider the option of keeping std::stringstream ?
> > > > >
> > > > > You mean formatting mostly with libfmt, but sometimes with streams? Or
> > > > > using libfmt but appending the formatted string to stringstream?
> > > >
> > > > Mostly using fmt::, but using std::stringstream without
> > > > fmt::memory_buffer when a string needs to be assembled. I'm not saying
> > > > it's necessarily a good idea, just wondering.
> > >
> > > Something I wrote which you can use to format C++ strings in a printf-style
> > >
> > > ```
> > >   #pragma once
> > >
> > >   #include <string.h>
> > >   #include <string>
> > >
> > >   // function that will sprintf to a C++ string starting from std::string::size()
> > >   // so if you want to completely overwrite a string or start at a specific point
> > >   // use std::string::clear() or std::string::resize(). str is a std::string.
> > >   #define STRING_PRINTF(str, ...)                                   \
> > >     do {                                                            \
> > >       const int size = snprintf(NULL, 0, __VA_ARGS__);              \
> > >       const size_t start_of_string = str.size();                    \
> > >       str.resize(start_of_string + size);                           \
> > >       snprintf(&str[start_of_string], str.size() + 1, __VA_ARGS__); \
> > >     } while (0)
> > > ```
> > >
> > > This way you can format C++ strings in a sprintf-style. It would give
> > > you many of the benefits of fmt with just a few lines of code
> > > alternatively. I know we don't only use macros, but it makes sense
> > > here because when the compiler see's the snprintf, it warns against
> > > incorrect format strings when you do it this way.
> >
> > Beside the fact that snprintf() is called twice which may be less
> > efficient (although maybe still more efficient than libfmt), the trouble
> > here is that __VA_ARGS__ will be evaluated twice.
> >
> >         unsigned int index;
> >         std::string str;
> >
> >         index = 0;
> >         STRING_PRINTF(str, "%u", index++);
> 
> These simple cases could use:
> 
>   std::string str = std::to_string(index++);

Of course. It was just an example of potential issues with multiple
evaluation of macro arguments. It should be possible to turn the macro
into a function instead, but I think we'll then end up reinventing the
wheel.

> It's unlikely that you are gonna beat std::to_string for a simple
> conversion like this (well, libfmt beats everything in terms of
> execution speed but it's another dependency, I've done a few
> benchmarks in the past). STRING_PRINTF would easily convert to
> std::format in future. The solution is efficient in terms of memory
> but will do an evaluation twice as you said. The above macro was
> written with readability and future compatibility to std::format in
> mind, and is not very different to what folly from Meta does (only
> much simpler and compiler friendly), see stringPrintf
> 
> https://github.com/facebook/folly/blob/main/folly/String.cpp
> 
> STRING_PRINTF suits the case where you have a longer format string and
> you want to do something a bit more complex, so something like:
> 
>         std::stringstream info;
>         info << ts / 1000000000 << "."
>              << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
>              << " (" << std::fixed << std::setprecision(2) << fps << " fps)";
> 
>         for (const auto &[stream, buffer] : buffers) {
>                 const FrameMetadata &metadata = buffer->metadata();
> 
>                 info << " " << streamNames_[stream]
>                      << " seq: " << std::setw(6) << std::setfill('0')
> << metadata.sequence
>                      << " bytesused: ";
> 
>                 unsigned int nplane = 0;
>                 for (const FrameMetadata::Plane &plane : metadata.planes()) {
>                         info << plane.bytesused;
>                         if (++nplane < metadata.planes().size())
>                                 info << "/";
>                 }
>         }
> 
>         if (sink_) {
>                 if (!sink_->processRequest(request))
>                         requeue = false;
>         }
> 
>         std::cout << info.str() << std::endl;
> 
> would become:
> 
>         std::string info
>         STRING_PRINTF(info, "%.6f (%.2f fps)", ts / 1000000000.0, fps);
> 
>         for (const auto &[stream, buffer] : buffers) {
>                 const FrameMetadata &metadata = buffer->metadata();
> 
>                 STRING_PRINTF(info,
>                              " %s seq: %06d bytesused: ", streamNames_[buf.first].c_str(),
>                              metadata.sequence);
> 
>                 unsigned int nplane = 0;
>                 for (const FrameMetadata::Plane &plane : metadata.planes()) {
>                         frame_str += std::to_string(plane.bytesused);
>                         if (++nplane < metadata.planes().size())
>                                 info += "/";
>                 }
>         }
> 
>         if (sink_) {
>                 if (!sink_->processRequest(request))
>                         requeue = false;
>         }
> 
>         printf("%s\n", info.c_str());
> 
> >         /* Now index is equal to 2 */
> >
> > > Just an option...

Eric Curtin May 23, 2022, 9:40 a.m. UTC | #19

On Mon, 23 May 2022 at 09:23, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Eric,
>
> On Sun, May 22, 2022 at 09:00:04PM +0100, Eric Curtin wrote:
> > On Sun, 22 May 2022 at 10:58, Laurent Pinchart wrote:
> > > On Sat, May 21, 2022 at 07:33:03PM +0100, Eric Curtin wrote:
> > > > On Tue, 10 May 2022 at 15:21, Laurent Pinchart wrote:
> > > > > On Tue, May 10, 2022 at 05:05:05PM +0300, Tomi Valkeinen wrote:
> > > > > > On 10/05/2022 16:22, Laurent Pinchart wrote:
> > > > > > > On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen 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've got it wrong. The first library you should add to any new C++
> > > > > > > project you make is libcamera :-)
> > > > > > >
> > > > > > >> 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.
> > > > > > >
> > > > > > > Is this because libfmt is a header-only library ? What if it is built as
> > > > > > > a shared object, what's the impact on code size in cam ?
> > > > > >
> > > > > > libfmt supports header-only, but it's not header-only here.
> > > > > >
> > > > > > >> Converting that code to use fmt::format() and
> > > > > > >> naive string append gives:
> > > > > > >>
> > > > > > >> release with string append 233680
> > > > > > >> release lto with string append     186936
> > > > > > >
> > > > > > > What's the impact of switching to string concatenation on runtime ?
> > > > > >
> > > > > > What do you mean?
> > > > >
> > > > > I mean what is the impact on CPU usage of using the "naive" string
> > > > > append, compared to std::stringstream or fmt::memory_buffer ?
> > > > >
> > > > > > > Would you consider the option of keeping std::stringstream ?
> > > > > >
> > > > > > You mean formatting mostly with libfmt, but sometimes with streams? Or
> > > > > > using libfmt but appending the formatted string to stringstream?
> > > > >
> > > > > Mostly using fmt::, but using std::stringstream without
> > > > > fmt::memory_buffer when a string needs to be assembled. I'm not saying
> > > > > it's necessarily a good idea, just wondering.
> > > >
> > > > Something I wrote which you can use to format C++ strings in a printf-style
> > > >
> > > > ```
> > > >   #pragma once
> > > >
> > > >   #include <string.h>
> > > >   #include <string>
> > > >
> > > >   // function that will sprintf to a C++ string starting from std::string::size()
> > > >   // so if you want to completely overwrite a string or start at a specific point
> > > >   // use std::string::clear() or std::string::resize(). str is a std::string.
> > > >   #define STRING_PRINTF(str, ...)                                   \
> > > >     do {                                                            \
> > > >       const int size = snprintf(NULL, 0, __VA_ARGS__);              \
> > > >       const size_t start_of_string = str.size();                    \
> > > >       str.resize(start_of_string + size);                           \
> > > >       snprintf(&str[start_of_string], str.size() + 1, __VA_ARGS__); \
> > > >     } while (0)
> > > > ```
> > > >
> > > > This way you can format C++ strings in a sprintf-style. It would give
> > > > you many of the benefits of fmt with just a few lines of code
> > > > alternatively. I know we don't only use macros, but it makes sense
> > > > here because when the compiler see's the snprintf, it warns against
> > > > incorrect format strings when you do it this way.
> > >
> > > Beside the fact that snprintf() is called twice which may be less
> > > efficient (although maybe still more efficient than libfmt), the trouble
> > > here is that __VA_ARGS__ will be evaluated twice.
> > >
> > >         unsigned int index;
> > >         std::string str;
> > >
> > >         index = 0;
> > >         STRING_PRINTF(str, "%u", index++);
> >
> > These simple cases could use:
> >
> >   std::string str = std::to_string(index++);
>
> Of course. It was just an example of potential issues with multiple
> evaluation of macro arguments. It should be possible to turn the macro
> into a function instead, but I think we'll then end up reinventing the
> wheel.

Yeah I get that, I'm not exactly hell bent on changing this, just
curiosity at this point (although admittedly I prefer printf or libfmt
style, and it's generally faster).

I did a crude benchmark here, just to solve curiosity:

https://github.com/ericcurtin/staging/blob/master/string_format_bench.cpp

$ g++ -O3 string_format_bench.cpp; ./a.out > /dev/null
folly: 3.447348 seconds
folly2: 3.343155 seconds
string_printf: 7.059757 seconds
string_printf2: 3.468643 seconds
to_string: 4.890018 seconds
ostringstream: 5.817828 seconds

string_printf2 takes some influence from folly and the first time it
parses, it optimistically guesses the output string could be less than
128 characters, which it probably is in many cases.

I actually wrote folly2 as a further optimization and opened a pull
request in Meta's folly, they missed a trick there by not writing
directly into the string, they do an extra unnecessary copy.

STRING_PRINTF2 is my favorite, but each to their own :)

>
> > It's unlikely that you are gonna beat std::to_string for a simple
> > conversion like this (well, libfmt beats everything in terms of
> > execution speed but it's another dependency, I've done a few
> > benchmarks in the past). STRING_PRINTF would easily convert to
> > std::format in future. The solution is efficient in terms of memory
> > but will do an evaluation twice as you said. The above macro was
> > written with readability and future compatibility to std::format in
> > mind, and is not very different to what folly from Meta does (only
> > much simpler and compiler friendly), see stringPrintf
> >
> > https://github.com/facebook/folly/blob/main/folly/String.cpp
> >
> > STRING_PRINTF suits the case where you have a longer format string and
> > you want to do something a bit more complex, so something like:
> >
> >         std::stringstream info;
> >         info << ts / 1000000000 << "."
> >              << std::setw(6) << std::setfill('0') << ts / 1000 % 1000000
> >              << " (" << std::fixed << std::setprecision(2) << fps << " fps)";
> >
> >         for (const auto &[stream, buffer] : buffers) {
> >                 const FrameMetadata &metadata = buffer->metadata();
> >
> >                 info << " " << streamNames_[stream]
> >                      << " seq: " << std::setw(6) << std::setfill('0')
> > << metadata.sequence
> >                      << " bytesused: ";
> >
> >                 unsigned int nplane = 0;
> >                 for (const FrameMetadata::Plane &plane : metadata.planes()) {
> >                         info << plane.bytesused;
> >                         if (++nplane < metadata.planes().size())
> >                                 info << "/";
> >                 }
> >         }
> >
> >         if (sink_) {
> >                 if (!sink_->processRequest(request))
> >                         requeue = false;
> >         }
> >
> >         std::cout << info.str() << std::endl;
> >
> > would become:
> >
> >         std::string info
> >         STRING_PRINTF(info, "%.6f (%.2f fps)", ts / 1000000000.0, fps);
> >
> >         for (const auto &[stream, buffer] : buffers) {
> >                 const FrameMetadata &metadata = buffer->metadata();
> >
> >                 STRING_PRINTF(info,
> >                              " %s seq: %06d bytesused: ", streamNames_[buf.first].c_str(),
> >                              metadata.sequence);
> >
> >                 unsigned int nplane = 0;
> >                 for (const FrameMetadata::Plane &plane : metadata.planes()) {
> >                         frame_str += std::to_string(plane.bytesused);
> >                         if (++nplane < metadata.planes().size())
> >                                 info += "/";
> >                 }
> >         }
> >
> >         if (sink_) {
> >                 if (!sink_->processRequest(request))
> >                         requeue = false;
> >         }
> >
> >         printf("%s\n", info.c_str());
> >
> > >         /* Now index is equal to 2 */
> > >
> > > > Just an option...
>
> --
> Regards,
>
> Laurent Pinchart
>

Tomi Valkeinen May 23, 2022, 11:19 a.m. UTC | #20

On 10/05/2022 17:20, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tue, May 10, 2022 at 05:05:05PM +0300, Tomi Valkeinen wrote:
>> On 10/05/2022 16:22, Laurent Pinchart wrote:
>>> On Tue, May 10, 2022 at 10:16:17AM +0300, Tomi Valkeinen 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've got it wrong. The first library you should add to any new C++
>>> project you make is libcamera :-)
>>>
>>>> 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.
>>>
>>> Is this because libfmt is a header-only library ? What if it is built as
>>> a shared object, what's the impact on code size in cam ?
>>
>> libfmt supports header-only, but it's not header-only here.
>>
>>>> Converting that code to use fmt::format() and
>>>> naive string append gives:
>>>>
>>>> release with string append	233680
>>>> release lto with string append	186936
>>>
>>> What's the impact of switching to string concatenation on runtime ?
>>
>> What do you mean?
> 
> I mean what is the impact on CPU usage of using the "naive" string
> append, compared to std::stringstream or fmt::memory_buffer ?
> 
>>> Would you consider the option of keeping std::stringstream ?
>>
>> You mean formatting mostly with libfmt, but sometimes with streams? Or
>> using libfmt but appending the formatted string to stringstream?
> 
> Mostly using fmt::, but using std::stringstream without
> fmt::memory_buffer when a string needs to be assembled. I'm not saying
> it's necessarily a good idea, just wondering.

I did test using fmt::format() and assembling the result to 
std::stringstream, and that does keep the executable size down.

So somehow fmt::format_to() seems to pull in a lot of code. I got a 
comment on libfmt gitter: "Normally format and format_to are compiled to 
the same underlying calls where the iterator is type erased.". I'm 
currently inclined to believe it's a bug.

  Tomi

Christian Rauch May 27, 2022, 11:19 p.m. UTC | #21

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?

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).

>
>> 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.
>

Eric Curtin May 30, 2022, 8:47 a.m. UTC | #22

On Sat, 28 May 2022 at 00:19, Christian Rauch via libcamera-devel
<libcamera-devel@lists.libcamera.org> 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?
>
> 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).
>
> >
> >> 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.

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

Laurent Pinchart May 30, 2022, 9:03 a.m. UTC | #23

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.

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

Christian Rauch May 30, 2022, 10:11 a.m. UTC | #24

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.
>

Laurent Pinchart May 30, 2022, 10:37 a.m. UTC | #25

Hi Christian,

On Mon, May 30, 2022 at 11:11:23AM +0100, Christian Rauch wrote:
> 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.

Not quite I'm afraid.

--------
#include <map>	/* This includes <optional> indirectly in libc++ */

using namespace std;

template<class T>
class optional
{
public:
        optional()
        {
        }
};

int main()
{
        optional<int> opt;

        return 0;
}
--------

$ clang++ -W -Wall -stdlib=libc++ -std=c++14 -o version version.cpp
$ clang++ -W -Wall -stdlib=libc++ -std=c++17 -o version version.cpp
version.cpp:16:2: error: reference to 'optional' is ambiguous
        optional<int> opt;
        ^
version.cpp:6:7: note: candidate found by name lookup is 'optional'
class optional
      ^
/usr/include/c++/v1/optional:590:7: note: candidate found by name lookup is 'std::optional'
class optional
      ^
1 error generated.

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

Christian Rauch May 30, 2022, 10:20 p.m. UTC | #26

Hi Laurent,

Am 30.05.22 um 11:37 schrieb Laurent Pinchart:
> Hi Christian,
>
> On Mon, May 30, 2022 at 11:11:23AM +0100, Christian Rauch wrote:
>> 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.
>
> Not quite I'm afraid.
>
> --------
> #include <map>	/* This includes <optional> indirectly in libc++ */
>
> using namespace std;
>
> template<class T>
> class optional
> {
> public:
>         optional()
>         {
>         }
> };
>
> int main()
> {
>         optional<int> opt;
>
>         return 0;
> }

Right, this is clashing because of ambiguous symbols from different
namespaces. You have a point there. I only thought in terms of API
compatibility there.

Anyway, libcamera already requires C++17 and uses std::optional. I would
like to push for a decision if libcamera:
A) continues to use C++17 and std::optional
B) reverts those changes and implements a custom "optional"

I am in favour of keeping the status quo of using std::optional. The
reason is that I think that not many projects (if any) will be affected
by these kinds of ambiguity issues above. Resorting to a custom
solution, just to switch to std::optional in the public API later
(when?), will cause more breakage issues. A custom temporary solution
will also require someone to implement and maintain this, knowing that
it will be replaced eventually anyway.

I simply think that the benefits outweigh the potential issues. Using
std::optional, for example as control value return type, improves the
descriptiveness of the API. While on the other side, there seem to be no
projects struggling with the current C++17/std::optional situation.

Sorry for pushing for a solution, but I really would like to continue
with my original patch set :-)

Best,
Christian

> --------
>
> $ clang++ -W -Wall -stdlib=libc++ -std=c++14 -o version version.cpp
> $ clang++ -W -Wall -stdlib=libc++ -std=c++17 -o version version.cpp
> version.cpp:16:2: error: reference to 'optional' is ambiguous
>         optional<int> opt;
>         ^
> version.cpp:6:7: note: candidate found by name lookup is 'optional'
> class optional
>       ^
> /usr/include/c++/v1/optional:590:7: note: candidate found by name lookup is 'std::optional'
> class optional
>       ^
> 1 error generated.
>
>>>> Given libcamera is still 0.0 it could be a nice opportunity to take.
>

Laurent Pinchart June 1, 2022, 8:59 a.m. UTC | #27

Hi Christian,

On Mon, May 30, 2022 at 11:20:06PM +0100, Christian Rauch via libcamera-devel wrote:
> Am 30.05.22 um 11:37 schrieb Laurent Pinchart:
> > On Mon, May 30, 2022 at 11:11:23AM +0100, Christian Rauch wrote:
> >> 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.
> >
> > Not quite I'm afraid.
> >
> > --------
> > #include <map>	/* This includes <optional> indirectly in libc++ */
> >
> > using namespace std;
> >
> > template<class T>
> > class optional
> > {
> > public:
> >         optional()
> >         {
> >         }
> > };
> >
> > int main()
> > {
> >         optional<int> opt;
> >
> >         return 0;
> > }
> 
> Right, this is clashing because of ambiguous symbols from different
> namespaces. You have a point there. I only thought in terms of API
> compatibility there.
> 
> Anyway, libcamera already requires C++17 and uses std::optional. I would
> like to push for a decision if libcamera:
> A) continues to use C++17 and std::optional
> B) reverts those changes and implements a custom "optional"
> 
> I am in favour of keeping the status quo of using std::optional. The
> reason is that I think that not many projects (if any) will be affected
> by these kinds of ambiguity issues above. Resorting to a custom
> solution, just to switch to std::optional in the public API later
> (when?), will cause more breakage issues. A custom temporary solution
> will also require someone to implement and maintain this, knowing that
> it will be replaced eventually anyway.
> 
> I simply think that the benefits outweigh the potential issues. Using
> std::optional, for example as control value return type, improves the
> descriptiveness of the API. While on the other side, there seem to be no
> projects struggling with the current C++17/std::optional situation.
> 
> Sorry for pushing for a solution, but I really would like to continue
> with my original patch set :-)

We may have trouble understanding each other. I'm fine with
std::optional being used in the public API for the time being. *If and
only if* we later realize C++17 is causing issues in some important use
cases, then we can switch to a custom utils::optional implementation.
The risk of having to do so seems low to me.

Tl;dr: More usage of std::optional in the public API is fine.

> > --------
> >
> > $ clang++ -W -Wall -stdlib=libc++ -std=c++14 -o version version.cpp
> > $ clang++ -W -Wall -stdlib=libc++ -std=c++17 -o version version.cpp
> > version.cpp:16:2: error: reference to 'optional' is ambiguous
> >         optional<int> opt;
> >         ^
> > version.cpp:6:7: note: candidate found by name lookup is 'optional'
> > class optional
> >       ^
> > /usr/include/c++/v1/optional:590:7: note: candidate found by name lookup is 'std::optional'
> > class optional
> >       ^
> > 1 error generated.
> >
> >>>> Given libcamera is still 0.0 it could be a nice opportunity to take.

[libcamera-devel] cam: convert to libfmt
diff mbox series

Commit Message

Comments

Patch
diff mbox series

[libcamera-devel] cam: convert to libfmt 15839 diff mbox series

Commit Message

Comments

Patch 15839 diff mbox series

[libcamera-devel] cam: convert to libfmt
diff mbox series

Patch
diff mbox series