[RFC,v2,13/13] apps: cam: Write raw file if PPM cannot be written
diff mbox series

Message ID 20250124215806.158024-14-mzamazal@redhat.com
State New
Headers show
Series
  • Enable raw streams with software ISP
Related show

Commit Message

Milan Zamazal Jan. 24, 2025, 9:58 p.m. UTC
`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(-)

Comments

Laurent Pinchart Jan. 25, 2025, 9:04 p.m. UTC | #1
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 |
Milan Zamazal Jan. 27, 2025, 12:13 p.m. UTC | #2
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 |
Laurent Pinchart Jan. 27, 2025, 5:45 p.m. UTC | #3
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 |

Patch
diff mbox series

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 |