| Message ID | 20250124215806.158024-14-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| 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 |
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > 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. Eventually, I decided to add new options to act at the stream level and keeping the original ones to act at the camera level. Hopefully to get the best of both. Unfortunately, it's more complicated (and more work) than I initially expected. I posted RFC patches, let's see. One bug fix and some cleanup included. >> 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. I rejected the idea because it doesn't match well with what happens with the stream keywords. > 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(-)