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

[Milan Zamazal]

Hi Laurent,

thank you for the review.

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

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

Done.

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

Done.

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

Done.

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

OK, done.  (My original motivation was to possibly cover other PNM formats but
PPM is the proper extension and as discussed in related threads, we probably
don't want to add much in this area.)

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

It's not supported everywhere, for example it's not recognized in Gqview or
Emacs (*bummer*) on my computer.  So better to stick with PPM.

> 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 ? :-)

I'm not sure whether DNG can support what we need.  In any case, it's associated
with "very" raw camera images and not much supported outside raw converters and
photo editors.

As for TIFF, the advantage of using PPM is that writing it is really trivial and
there is no need to use (or replicate) libtiff.  But yes, TIFF might be helpful
at least with non-RGB outputs.

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

Done.

>> +		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);
>> +};