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

[paul.elder at ideasonboard.com]

Hi Laurent,

On Tue, Oct 18, 2022 at 03:45:31AM +0300, Laurent Pinchart wrote:
> 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 ?

Yeah.

> 
> > 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 :-)

I'll keep the original :)

> 
> > +#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 ?

I don't like this.

qcam behaved like this, so I thought it was fine to just mimick it.

Eh, I guess I could expand it, shouldn't be *that* bad (famous last
words).

> 
> > +#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.

It's only needed for DNG writing, though. I guess it is good
future-proofing though :/


Paul

> 
> >  	~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_;
> >  };