[PATCH 2/2] apps: cam: Print an error when outputting DNG and DNG support is missing

[Milan Zamazal]

Hi Laurent,

thank you for the patch, nice improvement.

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> When DNG support is missing, the cam application ignores the .dng suffix
> of the file pattern and writes raw binary data instead, without
> notifying the user. This leads to confusion. Fix it by printing an error
> message.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Milan Zamazal <mzamazal at redhat.com>

> ---
>  src/apps/cam/camera_session.cpp | 15 ++++++---
>  src/apps/cam/file_sink.cpp      | 60 +++++++++++++++++++++++----------
>  src/apps/cam/file_sink.h        | 18 ++++++++--
>  3 files changed, 68 insertions(+), 25 deletions(-)
>
> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp
> index 097dc479241a..227df9e922ea 100644
> --- a/src/apps/cam/camera_session.cpp
> +++ b/src/apps/cam/camera_session.cpp
> @@ -230,11 +230,16 @@ int CameraSession::start()
>  #endif
>  
>  	if (options_.isSet(OptFile)) {
> -		if (!options_[OptFile].toString().empty())
> -			sink_ = std::make_unique<FileSink>(camera_.get(), streamNames_,
> -							   options_[OptFile]);
> -		else
> -			sink_ = std::make_unique<FileSink>(camera_.get(), streamNames_);
> +		std::unique_ptr<FileSink> sink =
> +			std::make_unique<FileSink>(camera_.get(), streamNames_);
> +
> +		if (!options_[OptFile].toString().empty()) {
> +			ret = sink->setFilePattern(options_[OptFile]);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		sink_ = std::move(sink);
>  	}
>  
>  	if (sink_) {
> diff --git a/src/apps/cam/file_sink.cpp b/src/apps/cam/file_sink.cpp
> index 3e000d2fd9c6..76e21db9bf9a 100644
> --- a/src/apps/cam/file_sink.cpp
> +++ b/src/apps/cam/file_sink.cpp
> @@ -5,6 +5,7 @@
>   * File Sink
>   */
>  
> +#include <array>
>  #include <assert.h>
>  #include <fcntl.h>
>  #include <iomanip>
> @@ -12,6 +13,7 @@
>  #include <sstream>
>  #include <string.h>
>  #include <unistd.h>
> +#include <utility>
>  
>  #include <libcamera/camera.h>
>  
> @@ -24,13 +26,13 @@
>  using namespace libcamera;
>  
>  FileSink::FileSink([[maybe_unused]] const libcamera::Camera *camera,
> -		   const std::map<const libcamera::Stream *, std::string> &streamNames,
> -		   const std::string &pattern)
> +		   const std::map<const libcamera::Stream *, std::string> &streamNames)
>  	:
>  #ifdef HAVE_TIFF
>  	  camera_(camera),
>  #endif
> -	  streamNames_(streamNames), pattern_(pattern)
> +	  pattern_(kDefaultFilePattern), fileType_(FileType::Binary),
> +	  streamNames_(streamNames)
>  {
>  }
>  
> @@ -38,6 +40,41 @@ FileSink::~FileSink()
>  {
>  }
>  
> +int FileSink::setFilePattern(const std::string &pattern)
> +{
> +	static const std::array<std::pair<std::string, FileType>, 2> types{{
> +		{ ".dng", FileType::Dng },
> +		{ ".ppm", FileType::Ppm },
> +	}};
> +
> +	pattern_ = pattern;
> +
> +	if (pattern_.empty() || pattern_.back() == '/')
> +		pattern_ += kDefaultFilePattern;
> +
> +	fileType_ = FileType::Binary;
> +
> +	for (const auto &type : types) {
> +		if (pattern_.size() < type.first.size())
> +			continue;
> +
> +		if (pattern_.find(type.first, pattern_.size() - type.first.size()) !=
> +		    std::string::npos) {
> +			fileType_ = type.second;
> +			break;
> +		}
> +	}
> +
> +#ifndef HAVE_TIFF
> +	if (fileType_ == FileType::Dng) {
> +		std::cerr << "DNG support not available" << std::endl;
> +		return -EINVAL;
> +	}
> +#endif /* HAVE_TIFF */
> +
> +	return 0;
> +}
> +
>  int FileSink::configure(const libcamera::CameraConfiguration &config)
>  {
>  	int ret = FrameSink::configure(config);
> @@ -67,21 +104,10 @@ bool FileSink::processRequest(Request *request)
>  void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
>  			   [[maybe_unused]] const ControlList &metadata)
>  {
> -	std::string filename;
> +	std::string filename = pattern_;
>  	size_t pos;
>  	int fd, ret = 0;
>  
> -	if (!pattern_.empty())
> -		filename = pattern_;
> -
> -#ifdef HAVE_TIFF
> -	bool dng = filename.find(".dng", filename.size() - 4) != std::string::npos;
> -#endif /* HAVE_TIFF */
> -	bool ppm = filename.find(".ppm", filename.size() - 4) != std::string::npos;
> -
> -	if (filename.empty() || filename.back() == '/')
> -		filename += "frame-#.bin";
> -
>  	pos = filename.find_first_of('#');
>  	if (pos != std::string::npos) {
>  		std::stringstream ss;
> @@ -93,7 +119,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
>  	Image *image = mappedBuffers_[buffer].get();
>  
>  #ifdef HAVE_TIFF
> -	if (dng) {
> +	if (fileType_ == FileType::Dng) {
>  		ret = DNGWriter::write(filename.c_str(), camera_,
>  				       stream->configuration(), metadata,
>  				       buffer, image->data(0).data());
> @@ -104,7 +130,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
>  		return;
>  	}
>  #endif /* HAVE_TIFF */
> -	if (ppm) {
> +	if (fileType_ == FileType::Ppm) {
>  		ret = PPMWriter::write(filename.c_str(), stream->configuration(),
>  				       image->data(0));
>  		if (ret < 0)
> diff --git a/src/apps/cam/file_sink.h b/src/apps/cam/file_sink.h
> index 9d560783af09..71b7fe0feab5 100644
> --- a/src/apps/cam/file_sink.h
> +++ b/src/apps/cam/file_sink.h
> @@ -21,10 +21,11 @@ class FileSink : public FrameSink
>  {
>  public:
>  	FileSink(const libcamera::Camera *camera,
> -		 const std::map<const libcamera::Stream *, std::string> &streamNames,
> -		 const std::string &pattern = "");
> +		 const std::map<const libcamera::Stream *, std::string> &streamNames);
>  	~FileSink();
>  
> +	int setFilePattern(const std::string &pattern);
> +
>  	int configure(const libcamera::CameraConfiguration &config) override;
>  
>  	void mapBuffer(libcamera::FrameBuffer *buffer) override;
> @@ -32,6 +33,14 @@ public:
>  	bool processRequest(libcamera::Request *request) override;
>  
>  private:
> +	static constexpr const char *kDefaultFilePattern = "frame-#.bin";
> +
> +	enum class FileType {
> +		Binary,
> +		Dng,
> +		Ppm,
> +	};
> +
>  	void writeBuffer(const libcamera::Stream *stream,
>  			 libcamera::FrameBuffer *buffer,
>  			 const libcamera::ControlList &metadata);
> @@ -39,7 +48,10 @@ private:
>  #ifdef HAVE_TIFF
>  	const libcamera::Camera *camera_;
>  #endif
> -	std::map<const libcamera::Stream *, std::string> streamNames_;
> +
>  	std::string pattern_;
> +	FileType fileType_;
> +
> +	std::map<const libcamera::Stream *, std::string> streamNames_;
>  	std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_;
>  };