[v3,1/1] apps: cam: Add support for pnm output format
diff mbox series

Message ID 20240318104357.441724-2-mzamazal@redhat.com
State New
Headers show
Series
  • cam: Add support for pnm output format
Related show

Commit Message

Milan Zamazal March 18, 2024, 10:43 a.m. UTC
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@redhat.com>
Reviewed-by: Kieran Bingham <kieran.bingham@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

Comments

Laurent Pinchart March 18, 2024, 12:40 p.m. UTC | #1
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@redhat.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@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);
> +};
Milan Zamazal March 22, 2024, 7:51 p.m. UTC | #2
Hi Laurent,

thank you for the review.

Laurent Pinchart <laurent.pinchart@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@redhat.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@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);
>> +};
Laurent Pinchart March 22, 2024, 8:53 p.m. UTC | #3
Hi Milan,

On Fri, Mar 22, 2024 at 08:51:17PM +0100, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > 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@redhat.com>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@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.

I was suspecting that. Fair enough.

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

I'd really want a file format that can support all the image formats we
need. TIFF could in theory support quite a few (but not all), but not
all of the features the file format supports are commonly used, so it
may end up not being very interoperable. Still, if all those features
were supported by a common conversion tool (e.g. imagemagick), it could
be a viable option.

The alternative is to create a universal format ourselves...

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

Patch
diff mbox series

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