[PATCH v2 1/1] apps: cam: Add support for pnm output format

[Kieran Bingham]

Hi Milan,

Quoting Milan Zamazal (2024-03-12 15:41:46)
> When file output is requested from cam app, it simply dumps the processed data
> and it must be converted to a readable image format manually.  Let's add support
> for pnm output file format to make files produced by cam directly readable by
> image display and processing software.
> 
> For now, only BGR888 output format, which is the simplest one to use, is
> supported but nothing prevents adding support for other output formats if
> needed.

What other formats are possible to add directly with PNM? Do we have to
do format conversions or can we write them directly?

https://www.fileformat.info/format/pbm/egff.htm looks like most other
formats will need converting to more or less RGB888.

I wish there was a way to 'standardise' outputting a header such as the
PPM form, but with a FOURCC to describe the data so we can output our
raw buffers but with a header ...

I wonder if we could sabotage/extend PNM/PPM in the future as a way of
storing V4L2/DRM formats : 

  (new format) (fourcc)
  P7 pRAA

Or at that point maybe we call it our own custom fileformat anyway ;-)



> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>  src/apps/cam/file_sink.cpp     | 11 +++++++
>  src/apps/cam/main.cpp          |  2 ++
>  src/apps/common/meson.build    |  1 +
>  src/apps/common/pnm_writer.cpp | 54 ++++++++++++++++++++++++++++++++++
>  src/apps/common/pnm_writer.h   | 18 ++++++++++++
>  5 files changed, 86 insertions(+)
>  create mode 100644 src/apps/common/pnm_writer.cpp
>  create mode 100644 src/apps/common/pnm_writer.h
> 
> diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp
> index dca350c4..3db59925 100644
> --- a/src/apps/cam/file_sink.cpp
> +++ b/src/apps/cam/file_sink.cpp
> @@ -16,6 +16,7 @@
>  #include <libcamera/camera.h>
>  
>  #include "../common/dng_writer.h"
> +#include "../common/pnm_writer.h"
>  #include "../common/image.h"
>  
>  #include "file_sink.h"
> @@ -73,6 +74,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
>         if (!pattern_.empty())
>                 filename = pattern_;
>  
> +       bool pnm = filename.find(".pnm", filename.size() - 4) != std::string::npos;
>  #ifdef HAVE_TIFF
>         bool dng = filename.find(".dng", filename.size() - 4) != std::string::npos;
>  #endif /* HAVE_TIFF */
> @@ -90,6 +92,15 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
>  
>         Image *image = mappedBuffers_[buffer].get();
>  
> +       if (pnm) {
> +               ret = PNMWriter::write(filename.c_str(), stream->configuration(),
> +                                      image->data(0).data());
> +               if (ret < 0)
> +                       std::cerr << "failed to write PNM file `" << filename
> +                                 << "'" << std::endl;
> +
> +               return;
> +       }
>  #ifdef HAVE_TIFF
>         if (dng) {
>                 ret = DNGWriter::write(filename.c_str(), camera_,
> diff --git a/src/apps/cam/main.cpp b/src/apps/cam/main.cpp
> index 179cc376..cdaa0baa 100644
> --- a/src/apps/cam/main.cpp
> +++ b/src/apps/cam/main.cpp
> @@ -150,6 +150,8 @@ int CamApp::parseOptions(int argc, char *argv[])
>                          "to write files, using the default file name. Otherwise it sets the\n"
>                          "full file path and name. The first '#' character in the file name\n"
>                          "is expanded to the camera index, stream name and frame sequence number.\n"
> +                        "If the file name ends with '.pnm', then the frame will be written to\n"
> +                        "the output file(s) in PNM format.\n"
>  #ifdef HAVE_TIFF
>                          "If the file name ends with '.dng', then the frame will be written to\n"
>                          "the output file(s) in DNG format.\n"
> diff --git a/src/apps/common/meson.build b/src/apps/common/meson.build
> index 479326cd..47b7e41a 100644
> --- a/src/apps/common/meson.build
> +++ b/src/apps/common/meson.build
> @@ -3,6 +3,7 @@
>  apps_sources = files([
>      'image.cpp',
>      'options.cpp',
> +    'pnm_writer.cpp',
>      'stream_options.cpp',
>  ])
>  
> diff --git a/src/apps/common/pnm_writer.cpp b/src/apps/common/pnm_writer.cpp
> new file mode 100644
> index 00000000..b505f97e
> --- /dev/null
> +++ b/src/apps/common/pnm_writer.cpp
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Red Hat, Inc.
> + *
> + * pnm_writer.cpp - PNM writer
> + */
> +
> +#include "pnm_writer.h"
> +
> +#include <fstream>
> +#include <iostream>
> +
> +#include <libcamera/formats.h>
> +#include <libcamera/pixel_format.h>
> +
> +using namespace libcamera;
> +
> +int PNMWriter::write(const char *filename,
> +                    const StreamConfiguration &config,
> +                    const void *data)

Shouldn't/couldn't the void *data be a Span? Or even an Image?

> +{
> +       if (config.pixelFormat != formats::BGR888) {

I sort of wonder if the outputs (FileSink etc) should expose their
handling as part of the pipeline configuration in cam, so that the
validation happens before the frames are captured. But honestly I don't
think it matters at this stage.

But I'd expect cam to be able to determine what capabilities are exposed
by the outputs and use that to filter the stream configuration to a
matching mode if one is not specified.

Still this will work for now as long as the full configuration is
accepted on both the camera and the PNMWriter...



> +               std::cerr << "Only BGR888 output pixel format is supported ("
> +                         << config.pixelFormat << " requested)" << std::endl;
> +               return -EINVAL;
> +       }
> +
> +       std::ofstream output(filename, std::ios::binary);
> +       if (!output) {
> +               std::cerr << "Failed to open pnm file: " << filename << std::endl;
> +               return -EINVAL;
> +       }
> +
> +       output << "P6" << std::endl
> +              << config.size.width << " " << config.size.height << std::endl
> +              << "255" << std::endl;
> +       if (!output) {
> +               std::cerr << "Failed to write the file header" << std::endl;
> +               return -EINVAL;
> +       }
> +
> +       const unsigned int rowLength = config.size.width * 3;
> +       const unsigned int paddedRowLength = config.stride;
> +       const char *row = reinterpret_cast<const char *>(data);
> +       for (unsigned int y = 0; y < config.size.height; y++, row += paddedRowLength) {
> +               output.write(row, rowLength);
> +               if (!output) {
> +                       std::cerr << "Failed to write image data at row " << y << std::endl;
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return 0;
> +}
> diff --git a/src/apps/common/pnm_writer.h b/src/apps/common/pnm_writer.h
> new file mode 100644
> index 00000000..a0ae4587
> --- /dev/null
> +++ b/src/apps/common/pnm_writer.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi Ltd
> + *

It looks like this should be fixed.


> + * pnm_writer.h - PNM writer
> + */
> +
> +#pragma once
> +
> +#include <libcamera/stream.h>
> +
> +class PNMWriter
> +{
> +public:
> +       static int write(const char *filename,
> +                        const libcamera::StreamConfiguration &config,
> +                        const void *data);

With the copyright fixed, and preferably a more scoped data variable
here:


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +};
> -- 
> 2.42.0
>