[PATCH v2 1/1] apps: cam: Add support for pnm output format
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Mar 18 12:25:55 CET 2024
Quoting Milan Zamazal (2024-03-18 10:44:59)
> 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?
Oh - absolutely, I agree the aim here is to be able to read the data
with standard tools. My comments are more about the future options.
Currently when we write out 'raw' binary frames - they are headerless.
So it can be difficult/awkward to determine what is in those raw frames.
The PPM header looks very easy to manage expressing what a raw buffer
contains, so I could imagine that a custom 'raw' frame writer could add
a header much alike this. Of course it wouldn't be readable by standard
tools ... but maybe we could propose something to extend 'standard'
tools to support the extensions in the future.
Things like https://git.retiisi.eu/?p=~sailus/raw2rgbpnm.git let us
convert raw captured frames, but we have to tell that tool exactly what
format was stored, and the correct size. A header would make that far
easier, as I frequently find it cumbersome to handle and essentially
have to have custom scripts for everything, which could otherwise be
solved with a binary raw format header.
--
Kieran
>
> >> 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