[libcamera-devel] [PATCH 2/3] cam: file_sink: Add support for DNG output

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tue, Oct 18, 2022 at 02:17:40AM +0900, Paul Elder via libcamera-devel wrote:
> Add support for outputting buffers in DNG format. It reuses the DNG
> writer that we had previously in qcam.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/cam/camera_session.cpp |  3 ++-
>  src/cam/file_sink.cpp      | 31 +++++++++++++++++++++++++------
>  src/cam/file_sink.h        |  7 +++++--
>  3 files changed, 32 insertions(+), 9 deletions(-)

Should the help text of the --file option be updated in main.cpp to
document this feature ?

> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> index 238186a3..2557f32d 100644
> --- a/src/cam/camera_session.cpp
> +++ b/src/cam/camera_session.cpp
> @@ -208,7 +208,8 @@ int CameraSession::start()
>  	if (options_.isSet(OptFile)) {
>  		if (!options_[OptFile].toString().empty())
>  			sink_ = std::make_unique<FileSink>(streamNames_,
> -							   options_[OptFile]);
> +							   options_[OptFile],
> +							   camera_.get());
>  		else
>  			sink_ = std::make_unique<FileSink>(streamNames_);
>  	}
> diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp
> index 45213d4a..fabf163c 100644
> --- a/src/cam/file_sink.cpp
> +++ b/src/cam/file_sink.cpp
> @@ -15,14 +15,15 @@
>  
>  #include <libcamera/camera.h>
>  
> +#include "dng_writer.h"
>  #include "file_sink.h"
>  #include "image.h"
>  
>  using namespace libcamera;
>  
>  FileSink::FileSink(const std::map<const libcamera::Stream *, std::string> &streamNames,
> -		   const std::string &pattern)
> -	: streamNames_(streamNames), pattern_(pattern)
> +		   const std::string &pattern, const libcamera::Camera *camera)
> +	: streamNames_(streamNames), pattern_(pattern), camera_(camera)
>  {
>  }
>  
> @@ -51,12 +52,13 @@ void FileSink::mapBuffer(FrameBuffer *buffer)
>  bool FileSink::processRequest(Request *request)
>  {
>  	for (auto [stream, buffer] : request->buffers())
> -		writeBuffer(stream, buffer);
> +		writeBuffer(stream, buffer, request->metadata());
>  
>  	return true;
>  }
>  
> -void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)
> +void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer,
> +			   [[maybe_unused]] const ControlList &metadata)
>  {
>  	std::string filename;
>  	size_t pos;
> @@ -65,6 +67,10 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)
>  	if (!pattern_.empty())
>  		filename = pattern_;
>  
> +#ifdef HAVE_TIFF
> +	bool dng = filename.find(".dng", filename.size() - 4) != std::string::npos;

I wish C++ had std::string::ends_width().

substr could also be used:

	bool dng = filename.substr(filename.size() - 4) == ".dng";

but that will result in a copy. It think std::string_view would solve
that:

	bool dng = std::string_view(filename).substr(filename.size() - 4) == ".dng";

but I'm not sure that's better :-)

> +#endif /* HAVE_TIFF */
> +
>  	if (filename.empty() || filename.back() == '/')
>  		filename += "frame-#.bin";
>  
> @@ -76,6 +82,21 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)
>  		filename.replace(pos, 1, ss.str());
>  	}
>  
> +	Image *image = mappedBuffers_[buffer].get();
> +
> +#ifdef HAVE_TIFF
> +	if (dng) {
> +		ret = DNGWriter::write(filename.c_str(), camera_,
> +				       stream->configuration(), metadata,
> +				       buffer, image->data(0).data());
> +		if (ret < 0)
> +			std::cerr << "failed to write DNG " << filename
> +				  << std::endl;
> +
> +		return;
> +	}

Hmmmm... You won't like this, but... what if the pipeline handler can
capture both raw and processed streams, and the user wants to write raw
frames in DNG and processed frames as .bin files ?

> +#endif /* HAVE_TIFF */
> +
>  	fd = open(filename.c_str(), O_CREAT | O_WRONLY |
>  		  (pos == std::string::npos ? O_APPEND : O_TRUNC),
>  		  S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH);
> @@ -86,8 +107,6 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)
>  		return;
>  	}
>  
> -	Image *image = mappedBuffers_[buffer].get();
> -
>  	for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
>  		const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
>  
> diff --git a/src/cam/file_sink.h b/src/cam/file_sink.h
> index 067736f5..e4838b37 100644
> --- a/src/cam/file_sink.h
> +++ b/src/cam/file_sink.h
> @@ -21,7 +21,7 @@ class FileSink : public FrameSink
>  {
>  public:
>  	FileSink(const std::map<const libcamera::Stream *, std::string> &streamNames,
> -		 const std::string &pattern = "");
> +		 const std::string &pattern = "", const libcamera::Camera *camera = nullptr);

I'd pass the camera as the first argument, to avoid having to add a
default value.

>  	~FileSink();
>  
>  	int configure(const libcamera::CameraConfiguration &config) override;
> @@ -32,9 +32,12 @@ public:
>  
>  private:
>  	void writeBuffer(const libcamera::Stream *stream,
> -			 libcamera::FrameBuffer *buffer);
> +			 libcamera::FrameBuffer *buffer,
> +			 const libcamera::ControlList &metadata);
>  
>  	std::map<const libcamera::Stream *, std::string> streamNames_;
>  	std::string pattern_;
>  	std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_;
> +
> +	const libcamera::Camera *camera_;
>  };

-- 
Regards,

Laurent Pinchart