Message ID | 20250124215806.158024-14-mzamazal@redhat.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Hi Milan, Thank you for the patch. On Fri, Jan 24, 2025 at 10:58:04PM +0100, Milan Zamazal wrote: > `cam' application can be requested to write its output as a PPM file. > However, this is supported only for certain formats (only BGR888 > currently). If the output format cannot be written, `cam' reports an > error and doesn't write anything. > > This is all right with a single stream input but impractical with > multiple streams of different output formats (e.g. a processed stream + > a raw stream) where some of them can be written in the supported format > while the other not. In such a case, it's better to write the supported > formats as PPM files and the unsupported formats as raw files, with the > corresponding warning on stderr. `.raw' extension is added to the file > name in such a case to make clear it's not a PPM file. > > This is a sort of hack but serves the purpose for the moment. Should we instead change the -F option (and the -D and -S options too I suppose) to act at the stream level instead of the camera level ? > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/apps/cam/file_sink.cpp | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp > index 76e21db9..1a866137 100644 > --- a/src/apps/cam/file_sink.cpp > +++ b/src/apps/cam/file_sink.cpp > @@ -7,6 +7,7 @@ > > #include <array> > #include <assert.h> > +#include <errno.h> > #include <fcntl.h> > #include <iomanip> > #include <iostream> > @@ -133,11 +134,16 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer, > if (fileType_ == FileType::Ppm) { > ret = PPMWriter::write(filename.c_str(), stream->configuration(), > image->data(0)); > - if (ret < 0) > - std::cerr << "failed to write PPM file `" << filename > - << "'" << std::endl; > - > - return; > + if (ret == -EINVAL) { > + filename += ".raw"; > + std::cerr << "cannot write file in PPM format, writing `" > + << filename << "' instead" << std::endl; > + } else { > + if (ret < 0) > + std::cerr << "failed to write PPM file `" << filename > + << "'" << std::endl; > + return; > + } > } > > fd = open(filename.c_str(), O_CREAT | O_WRONLY |
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Fri, Jan 24, 2025 at 10:58:04PM +0100, Milan Zamazal wrote: >> `cam' application can be requested to write its output as a PPM file. >> However, this is supported only for certain formats (only BGR888 >> currently). If the output format cannot be written, `cam' reports an >> error and doesn't write anything. >> >> This is all right with a single stream input but impractical with >> multiple streams of different output formats (e.g. a processed stream + >> a raw stream) where some of them can be written in the supported format >> while the other not. In such a case, it's better to write the supported >> formats as PPM files and the unsupported formats as raw files, with the >> corresponding warning on stderr. `.raw' extension is added to the file >> name in such a case to make clear it's not a PPM file. >> >> This is a sort of hack but serves the purpose for the moment. > > Should we instead change the -F option (and the -D and -S options too I > suppose) to act at the stream level instead of the camera level ? Something like this would be useful. But maybe rather than changing the given options (how?), it would be simpler to add a keyword to -s, e.g. output=file/sdl/kms. What do you think? >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/apps/cam/file_sink.cpp | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp >> index 76e21db9..1a866137 100644 >> --- a/src/apps/cam/file_sink.cpp >> +++ b/src/apps/cam/file_sink.cpp >> @@ -7,6 +7,7 @@ >> >> #include <array> >> #include <assert.h> >> +#include <errno.h> >> #include <fcntl.h> >> #include <iomanip> >> #include <iostream> >> @@ -133,11 +134,16 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer, >> if (fileType_ == FileType::Ppm) { >> ret = PPMWriter::write(filename.c_str(), stream->configuration(), >> image->data(0)); >> - if (ret < 0) >> - std::cerr << "failed to write PPM file `" << filename >> - << "'" << std::endl; >> - >> - return; >> + if (ret == -EINVAL) { >> + filename += ".raw"; >> + std::cerr << "cannot write file in PPM format, writing `" >> + << filename << "' instead" << std::endl; >> + } else { >> + if (ret < 0) >> + std::cerr << "failed to write PPM file `" << filename >> + << "'" << std::endl; >> + return; >> + } >> } >> >> fd = open(filename.c_str(), O_CREAT | O_WRONLY |
On Mon, Jan 27, 2025 at 01:13:40PM +0100, Milan Zamazal wrote: > Laurent Pinchart writes: > > On Fri, Jan 24, 2025 at 10:58:04PM +0100, Milan Zamazal wrote: > >> `cam' application can be requested to write its output as a PPM file. > >> However, this is supported only for certain formats (only BGR888 > >> currently). If the output format cannot be written, `cam' reports an > >> error and doesn't write anything. > >> > >> This is all right with a single stream input but impractical with > >> multiple streams of different output formats (e.g. a processed stream + > >> a raw stream) where some of them can be written in the supported format > >> while the other not. In such a case, it's better to write the supported > >> formats as PPM files and the unsupported formats as raw files, with the > >> corresponding warning on stderr. `.raw' extension is added to the file > >> name in such a case to make clear it's not a PPM file. > >> > >> This is a sort of hack but serves the purpose for the moment. > > > > Should we instead change the -F option (and the -D and -S options too I > > suppose) to act at the stream level instead of the camera level ? > > Something like this would be useful. But maybe rather than changing the > given options (how?), The OptionParser class supports hierarchical options, the addOption() function takes a parent as its last argument. > it would be simpler to add a keyword to -s, > e.g. output=file/sdl/kms. What do you think? That's an interesting idea too. Should we then deprecate the -F, -D and -S options ? I'm a bit concerned about how disturbing that would be. > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/apps/cam/file_sink.cpp | 16 +++++++++++----- > >> 1 file changed, 11 insertions(+), 5 deletions(-) > >> > >> diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp > >> index 76e21db9..1a866137 100644 > >> --- a/src/apps/cam/file_sink.cpp > >> +++ b/src/apps/cam/file_sink.cpp > >> @@ -7,6 +7,7 @@ > >> > >> #include <array> > >> #include <assert.h> > >> +#include <errno.h> > >> #include <fcntl.h> > >> #include <iomanip> > >> #include <iostream> > >> @@ -133,11 +134,16 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer, > >> if (fileType_ == FileType::Ppm) { > >> ret = PPMWriter::write(filename.c_str(), stream->configuration(), > >> image->data(0)); > >> - if (ret < 0) > >> - std::cerr << "failed to write PPM file `" << filename > >> - << "'" << std::endl; > >> - > >> - return; > >> + if (ret == -EINVAL) { > >> + filename += ".raw"; > >> + std::cerr << "cannot write file in PPM format, writing `" > >> + << filename << "' instead" << std::endl; > >> + } else { > >> + if (ret < 0) > >> + std::cerr << "failed to write PPM file `" << filename > >> + << "'" << std::endl; > >> + return; > >> + } > >> } > >> > >> fd = open(filename.c_str(), O_CREAT | O_WRONLY |
diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp index 76e21db9..1a866137 100644 --- a/src/apps/cam/file_sink.cpp +++ b/src/apps/cam/file_sink.cpp @@ -7,6 +7,7 @@ #include <array> #include <assert.h> +#include <errno.h> #include <fcntl.h> #include <iomanip> #include <iostream> @@ -133,11 +134,16 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer, if (fileType_ == FileType::Ppm) { ret = PPMWriter::write(filename.c_str(), stream->configuration(), image->data(0)); - if (ret < 0) - std::cerr << "failed to write PPM file `" << filename - << "'" << std::endl; - - return; + if (ret == -EINVAL) { + filename += ".raw"; + std::cerr << "cannot write file in PPM format, writing `" + << filename << "' instead" << std::endl; + } else { + if (ret < 0) + std::cerr << "failed to write PPM file `" << filename + << "'" << std::endl; + return; + } } fd = open(filename.c_str(), O_CREAT | O_WRONLY |
`cam' application can be requested to write its output as a PPM file. However, this is supported only for certain formats (only BGR888 currently). If the output format cannot be written, `cam' reports an error and doesn't write anything. This is all right with a single stream input but impractical with multiple streams of different output formats (e.g. a processed stream + a raw stream) where some of them can be written in the supported format while the other not. In such a case, it's better to write the supported formats as PPM files and the unsupported formats as raw files, with the corresponding warning on stderr. `.raw' extension is added to the file name in such a case to make clear it's not a PPM file. This is a sort of hack but serves the purpose for the moment. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/apps/cam/file_sink.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)