[libcamera-devel,2/3] cam: file_sink: Add support for DNG output
diff mbox series

Message ID 20221017171741.3803909-3-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • cam: Add DNG support
Related show

Commit Message

Paul Elder Oct. 17, 2022, 5:17 p.m. UTC 
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@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(-)

Comments

Laurent Pinchart Oct. 18, 2022, 12:45 a.m. UTC | #1 
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@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_;
>  };
Nicolas Dufresne via libcamera-devel Oct. 18, 2022, 6:46 a.m. UTC | #2 
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@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_;
> >  };

Patch
diff mbox series 

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