| Message ID | 20240312154146.1135416-2-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, 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. 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 ;-) > Signed-off-by: Milan Zamazal <mzamazal@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? > +{ > + 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. Still this will work for now as long as the full configuration is accepted on both the camera and the PNMWriter... > + 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. > + * 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@ideasonboard.com> > +}; > -- > 2.42.0 >
Hi Kieran, thank you for the review. Kieran Bingham <kieran.bingham@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@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@ideasonboard.com> > >> +}; >> -- >> 2.42.0 >>
Quoting Milan Zamazal (2024-03-18 10:44:59) > Hi Kieran, > > thank you for the review. > > Kieran Bingham <kieran.bingham@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@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@ideasonboard.com> > > > >> +}; > >> -- > >> 2.42.0 > >> >
Hello, On Mon, Mar 18, 2024 at 11:25:55AM +0000, Kieran Bingham wrote: > Quoting Milan Zamazal (2024-03-18 10:44:59) > > Kieran Bingham <kieran.bingham@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. What we need here, I think, is a standardized file format that can store a raw image with enough metadata to allow interoperability between applications. I don't think conversion and encoding to other formats is a good idea, as it will quickly get out of control, and isn't the purpose of cam. I'm fine adding PPM support when the data is already in RGB888 format as a quick workaround though, until we have something better, but I don't want to expand it with format conversion. I haven't researched this problem area in great details. One possibly interesting option starting point for the metadata related to the format description is the Khronos Data Format Specification ([1]). It's missing a colour model for raw Bayer data, but we could work with Khronos to add it. Regarding file formats, there's the Khronos Texture specification ([2]), but that uses Vulkan formats and is likely too specific to the needs of textures (although we should still check it, just in case). Is anyone aware of a standard format we could use ? [1] https://www.khronos.org/dataformat [2] https://www.khronos.org/ktx/ > > > 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. I agree, and that's the direction I'd like to take. If we can find an existing standard format suitable to store raw frames, and readable by existing tools, that's great. If we can't, we can create one, and work on getting conversion tools such as raw2rgbpnm to understand it. > > >> Signed-off-by: Milan Zamazal <mzamazal@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. It would be useful for the display sink too, so that cam would configure the stream with a supported format automatically (unless overridden by the -s argument). > > > > > > 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@ideasonboard.com> > > > > > >> +};
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) +{ + 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); + 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 + * + * 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); +};
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> --- 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