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

[Laurent Pinchart]

Hi Milan,

Thank you for the patch.

On Mon, Mar 18, 2024 at 11:43:57AM +0100, Milan Zamazal wrote:
> 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.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.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   | 20 +++++++++++++
>  5 files changed, 88 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..ac53719d 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;

Could you move this after the HAVE_TIFF section, to sort the formats
alphabetically ?

>  #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));
> +		if (ret < 0)
> +			std::cerr << "failed to write PNM file `" << filename
> +				  << "'" << std::endl;
> +
> +		return;
> +	}

Same here.

>  #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"

And here.

>  #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..9a63ec05
> --- /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 Span<uint8_t> &data)
> +{
> +	if (config.pixelFormat != formats::BGR888) {
> +		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

P6 refers to the Portable PixMap format, whose recommended extension is
.ppm. Some tools have trouble opening PPM files when named with a .pnm
suffix. I would use a .ppm file extension.

I'm also wondering if we shouldn't switch to the PAM format already (P7,
see https://en.wikipedia.org/wiki/Portable_anymap#PAM_graphics_format)
as it also supports greyscale formats. It's not the universal raw file
format I'm looking for though, and maybe not worth it (I haven't checked
how well supported PAM is).

For the sake of completeness, I want to mention we have a DNG writer
that outputs DNG files, and DNG is a TIFF file under the hood. TIFF
supports a set of RGB and YCbCr formats too... Opinions ? :-)

> +	       << 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.data());
> +	for (unsigned int y = 0; y < config.size.height; y++, row += paddedRowLength) {

You could use config.stride directly here, and drop 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..6c1e7967
> --- /dev/null
> +++ b/src/apps/common/pnm_writer.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat, Inc.
> + *
> + * pnm_writer.h - PNM writer
> + */
> +
> +#pragma once
> +
> +#include <libcamera/base/span.h>
> +
> +#include <libcamera/stream.h>
> +
> +class PNMWriter
> +{
> +public:
> +	static int write(const char *filename,
> +			 const libcamera::StreamConfiguration &config,
> +			 const libcamera::Span<uint8_t> &data);
> +};

-- 
Regards,

Laurent Pinchart