[PATCH v2 1/1] apps: cam: Add support for pnm output format
Milan Zamazal
mzamazal at redhat.com
Mon Mar 18 11:44:59 CET 2024
Hi Kieran,
thank you for the review.
Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> 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.
Yes, the same as with other popular formats.
> 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 ;-)
The idea of this patch is to be able to read or process the resulting image
directly with common image viewers and processing tools. What would be the use
of a custom format? We would need something to convert it to a common format
anyway and then we can omit the custom format as an intermediary step, unless
it's needed for speed or something, but would there be a real need for something
like that?
>> 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?
Yes, I think using Span here is better, done in v3.
>> +{
>> + 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.
This would be definitely good. Especially because when taking multiple frames,
the current version captures them all and fails for all of them.
> Still this will work for now as long as the full configuration is
> accepted on both the camera and the PNMWriter...
Yes. If the choice is between having this now or maybe a better version in the
future, I'd vote for having this now and maybe a better version in the future. :-)
>> + 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.
Right, done in v3.
>> + * 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
>>
More information about the libcamera-devel
mailing list