[libcamera-devel,v3,3/3] qcam: Add RAW capture support
diff mbox series

Message ID 20200502021045.785979-4-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • qcam: Add RAW capture support
Related show

Commit Message

Niklas Söderlund May 2, 2020, 2:10 a.m. UTC 
Add a toolbar button that captures RAW data to disk. The button is only
enabled if the camera is configured to provide a raw stream to the
application.

Only when the capture action is triggered will a request with a raw
buffer be queued to the camera.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
* Changes since v2
- Use a file dialog
- Make DNG optional depending on libtiff
---
 src/qcam/assets/feathericons/feathericons.qrc |  1 +
 src/qcam/main_window.cpp                      | 64 ++++++++++++++++++-
 src/qcam/main_window.h                        |  4 ++
 3 files changed, 68 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart May 2, 2020, 2:38 a.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Sat, May 02, 2020 at 04:10:45AM +0200, Niklas Söderlund wrote:
> Add a toolbar button that captures RAW data to disk. The button is only
> enabled if the camera is configured to provide a raw stream to the
> application.
> 
> Only when the capture action is triggered will a request with a raw
> buffer be queued to the camera.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * Changes since v2
> - Use a file dialog
> - Make DNG optional depending on libtiff
> ---
>  src/qcam/assets/feathericons/feathericons.qrc |  1 +
>  src/qcam/main_window.cpp                      | 64 ++++++++++++++++++-
>  src/qcam/main_window.h                        |  4 ++
>  3 files changed, 68 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc
> index c4eb7a0be6884373..fc8213928ece70ea 100644
> --- a/src/qcam/assets/feathericons/feathericons.qrc
> +++ b/src/qcam/assets/feathericons/feathericons.qrc
> @@ -1,5 +1,6 @@
>  <!DOCTYPE RCC><RCC version="1.0">
>  <qresource>
> +<file>./aperture.svg</file>
>  <file>./camera-off.svg</file>
>  <file>./play-circle.svg</file>
>  <file>./save.svg</file>
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 0bd9f3583ea4f6d4..458da479a9b21b73 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -27,6 +27,10 @@
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/version.h>
>  
> +#if HAVE_TIFF

I wonder if this shouldn't be called HAVE_DNG, as the dependency on
libtiff is internal to the DNGWriter class.

> +#include "dng_writer.h"
> +#endif
> +
>  using namespace libcamera;
>  
>  /**
> @@ -48,7 +52,8 @@ public:
>  };
>  
>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> -	: options_(options), cm_(cm), allocator_(nullptr), isCapturing_(false)
> +	: saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr),
> +	  isCapturing_(false), captureRaw_(false)
>  {
>  	int ret;
>  
> @@ -144,6 +149,16 @@ int MainWindow::createToolbars()
>  	action->setShortcut(QKeySequence::SaveAs);
>  	connect(action, &QAction::triggered, this, &MainWindow::saveImageAs);
>  
> +#if HAVE_TIFF
> +	/* Save Raw action. */
> +	action = toolbar_->addAction(QIcon::fromTheme("camera-photo",
> +						      QIcon(":aperture.svg")),
> +				     "Save Raw");
> +	action->setEnabled(false);
> +	connect(action, &QAction::triggered, this, &MainWindow::captureRaw);
> +	saveRaw_ = action;
> +#endif

I wonder if we should still have the button, but never enable it. If we
don't, users may wonder why the button isn't there. But if we do, they
may wonder why it's never enabled :-)

Maybe the tooltip text could be set to

- "RAW capture not available (missing libtiff)"
- "RAW capture not enabled on the command line"
- "Save RAW"

depending on the situation. Or maybe all of this is overkill :-)

> +
>  	return 0;
>  }
>  
> @@ -369,6 +384,10 @@ int MainWindow::startCapture()
>  
>  	adjustSize();
>  
> +	/* Configure the raw capture button. */
> +	if (saveRaw_)
> +		saveRaw_->setEnabled(config_->size() == 2);
> +
>  	/* Allocate and map buffers. */
>  	allocator_ = new FrameBufferAllocator(camera_);
>  	for (StreamConfiguration &config : *config_) {
> @@ -474,6 +493,9 @@ void MainWindow::stopCapture()
>  		return;
>  
>  	viewfinder_->stop();
> +	if (saveRaw_)
> +		saveRaw_->setEnabled(false);
> +	captureRaw_ = false;
>  
>  	int ret = camera_->stop();
>  	if (ret)
> @@ -524,6 +546,31 @@ void MainWindow::saveImageAs()
>  	writer.write(image);
>  }
>  
> +void MainWindow::captureRaw()
> +{
> +	captureRaw_ = true;
> +}
> +
> +void MainWindow::processRaw(FrameBuffer *buffer)
> +{
> +#if HAVE_TIFF
> +	QString defaultPath = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation);
> +	QString filename = QFileDialog::getSaveFileName(this, "Save DNG", defaultPath,
> +							"DNG Files (*.dng)");
> +
> +	if (filename != "") {

	if (!filename.isEmpty()) {

> +		const MappedBuffer &mapped = mappedBuffers_[buffer];
> +		DNGWriter::write(filename.toStdString().c_str(), camera_.get(),
> +				 rawStream_->configuration(), buffer,
> +				 mapped.memory);
> +	}
> +#endif

Blank line here ?

> +	{
> +		QMutexLocker locker(&mutex_);
> +		freeBuffers_[rawStream_].enqueue(buffer);
> +	}
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Request Completion Handling
>   */
> @@ -566,6 +613,9 @@ void MainWindow::processCapture()
>  	/* Process buffers. */
>  	if (buffers.count(vfStream_))
>  		processViewfinder(buffers[vfStream_]);
> +
> +	if (buffers.count(rawStream_))
> +		processRaw(buffers[rawStream_]);
>  }
>  
>  void MainWindow::processViewfinder(FrameBuffer *buffer)
> @@ -598,5 +648,17 @@ void MainWindow::queueRequest(FrameBuffer *buffer)
>  
>  	request->addBuffer(vfStream_, buffer);
>  
> +	if (captureRaw_) {
> +		QMutexLocker locker(&mutex_);
> +
> +		if (!freeBuffers_[rawStream_].isEmpty()) {
> +			request->addBuffer(rawStream_,
> +					   freeBuffers_[rawStream_].dequeue());
> +			captureRaw_ = false;
> +		} else {
> +			qWarning() << "No free buffer available for RAW capture";
> +		}
> +	}

We can minimize the amount of code covered by the lock.

	if (captureRaw_) {
		FrameBuffer *buffer;

		{
			QMutexLocker locker(&mutex_);
			buffer = !freeBuffers_[rawStream_].isEmpty())
			       ? freeBuffers_[rawStream_].dequeue() : nullptr;
		}

		if (buffer) {
			request->addBuffer(rawStream_, buffer);
			captureRaw_ = false;
		} else {
			qWarning() << "No free buffer available for RAW capture";
		}
	}

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  	camera_->queueRequest(request);
>  }
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 4856ecc10729159c..295ecc537e9d45bf 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -55,6 +55,8 @@ private Q_SLOTS:
>  	void toggleCapture(bool start);
>  
>  	void saveImageAs();
> +	void captureRaw();
> +	void processRaw(FrameBuffer *buffer);
>  
>  	void queueRequest(FrameBuffer *buffer);
>  
> @@ -75,6 +77,7 @@ private:
>  	QToolBar *toolbar_;
>  	QAction *startStopAction_;
>  	QComboBox *cameraCombo_;
> +	QAction *saveRaw_;
>  	ViewFinder *viewfinder_;
>  
>  	QIcon iconPlay_;
> @@ -96,6 +99,7 @@ private:
>  
>  	/* Capture state, buffers queue and statistics */
>  	bool isCapturing_;
> +	bool captureRaw_;
>  	Stream *vfStream_;
>  	Stream *rawStream_;
>  	std::map<Stream *, QQueue<FrameBuffer *>> freeBuffers_;
Niklas Söderlund May 2, 2020, 11:52 a.m. UTC | #2 
Hi Laurent,

Thanks for your feedback.

On 2020-05-02 05:38:16 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sat, May 02, 2020 at 04:10:45AM +0200, Niklas Söderlund wrote:
> > Add a toolbar button that captures RAW data to disk. The button is only
> > enabled if the camera is configured to provide a raw stream to the
> > application.
> > 
> > Only when the capture action is triggered will a request with a raw
> > buffer be queued to the camera.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> > * Changes since v2
> > - Use a file dialog
> > - Make DNG optional depending on libtiff
> > ---
> >  src/qcam/assets/feathericons/feathericons.qrc |  1 +
> >  src/qcam/main_window.cpp                      | 64 ++++++++++++++++++-
> >  src/qcam/main_window.h                        |  4 ++
> >  3 files changed, 68 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc
> > index c4eb7a0be6884373..fc8213928ece70ea 100644
> > --- a/src/qcam/assets/feathericons/feathericons.qrc
> > +++ b/src/qcam/assets/feathericons/feathericons.qrc
> > @@ -1,5 +1,6 @@
> >  <!DOCTYPE RCC><RCC version="1.0">
> >  <qresource>
> > +<file>./aperture.svg</file>
> >  <file>./camera-off.svg</file>
> >  <file>./play-circle.svg</file>
> >  <file>./save.svg</file>
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 0bd9f3583ea4f6d4..458da479a9b21b73 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -27,6 +27,10 @@
> >  #include <libcamera/camera_manager.h>
> >  #include <libcamera/version.h>
> >  
> > +#if HAVE_TIFF
> 
> I wonder if this shouldn't be called HAVE_DNG, as the dependency on
> libtiff is internal to the DNGWriter class.

I have reworked this to always include dng_writer.h here. And then set 
HAVE_DNG in dng_writer.h if HAVE_TIFF is set (by meson).

> 
> > +#include "dng_writer.h"
> > +#endif
> > +
> >  using namespace libcamera;
> >  
> >  /**
> > @@ -48,7 +52,8 @@ public:
> >  };
> >  
> >  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> > -	: options_(options), cm_(cm), allocator_(nullptr), isCapturing_(false)
> > +	: saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr),
> > +	  isCapturing_(false), captureRaw_(false)
> >  {
> >  	int ret;
> >  
> > @@ -144,6 +149,16 @@ int MainWindow::createToolbars()
> >  	action->setShortcut(QKeySequence::SaveAs);
> >  	connect(action, &QAction::triggered, this, &MainWindow::saveImageAs);
> >  
> > +#if HAVE_TIFF
> > +	/* Save Raw action. */
> > +	action = toolbar_->addAction(QIcon::fromTheme("camera-photo",
> > +						      QIcon(":aperture.svg")),
> > +				     "Save Raw");
> > +	action->setEnabled(false);
> > +	connect(action, &QAction::triggered, this, &MainWindow::captureRaw);
> > +	saveRaw_ = action;
> > +#endif
> 
> I wonder if we should still have the button, but never enable it. If we
> don't, users may wonder why the button isn't there. But if we do, they
> may wonder why it's never enabled :-)
> 
> Maybe the tooltip text could be set to
> 
> - "RAW capture not available (missing libtiff)"
> - "RAW capture not enabled on the command line"
> - "Save RAW"
> 
> depending on the situation. Or maybe all of this is overkill :-)

I think this is overkill and have kept it as is. My reasoning is that we 
shall not clutter the GUI with buttons for features the user have opted 
to not have. Also I think making this code more complex is a bad idea as 
we already know we need to redisgen the whole UI to support multiple 
streams properly.

> 
> > +
> >  	return 0;
> >  }
> >  
> > @@ -369,6 +384,10 @@ int MainWindow::startCapture()
> >  
> >  	adjustSize();
> >  
> > +	/* Configure the raw capture button. */
> > +	if (saveRaw_)
> > +		saveRaw_->setEnabled(config_->size() == 2);
> > +
> >  	/* Allocate and map buffers. */
> >  	allocator_ = new FrameBufferAllocator(camera_);
> >  	for (StreamConfiguration &config : *config_) {
> > @@ -474,6 +493,9 @@ void MainWindow::stopCapture()
> >  		return;
> >  
> >  	viewfinder_->stop();
> > +	if (saveRaw_)
> > +		saveRaw_->setEnabled(false);
> > +	captureRaw_ = false;
> >  
> >  	int ret = camera_->stop();
> >  	if (ret)
> > @@ -524,6 +546,31 @@ void MainWindow::saveImageAs()
> >  	writer.write(image);
> >  }
> >  
> > +void MainWindow::captureRaw()
> > +{
> > +	captureRaw_ = true;
> > +}
> > +
> > +void MainWindow::processRaw(FrameBuffer *buffer)
> > +{
> > +#if HAVE_TIFF
> > +	QString defaultPath = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation);
> > +	QString filename = QFileDialog::getSaveFileName(this, "Save DNG", defaultPath,
> > +							"DNG Files (*.dng)");
> > +
> > +	if (filename != "") {
> 
> 	if (!filename.isEmpty()) {
> 
> > +		const MappedBuffer &mapped = mappedBuffers_[buffer];
> > +		DNGWriter::write(filename.toStdString().c_str(), camera_.get(),
> > +				 rawStream_->configuration(), buffer,
> > +				 mapped.memory);
> > +	}
> > +#endif
> 
> Blank line here ?
> 
> > +	{
> > +		QMutexLocker locker(&mutex_);
> > +		freeBuffers_[rawStream_].enqueue(buffer);
> > +	}
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Request Completion Handling
> >   */
> > @@ -566,6 +613,9 @@ void MainWindow::processCapture()
> >  	/* Process buffers. */
> >  	if (buffers.count(vfStream_))
> >  		processViewfinder(buffers[vfStream_]);
> > +
> > +	if (buffers.count(rawStream_))
> > +		processRaw(buffers[rawStream_]);
> >  }
> >  
> >  void MainWindow::processViewfinder(FrameBuffer *buffer)
> > @@ -598,5 +648,17 @@ void MainWindow::queueRequest(FrameBuffer *buffer)
> >  
> >  	request->addBuffer(vfStream_, buffer);
> >  
> > +	if (captureRaw_) {
> > +		QMutexLocker locker(&mutex_);
> > +
> > +		if (!freeBuffers_[rawStream_].isEmpty()) {
> > +			request->addBuffer(rawStream_,
> > +					   freeBuffers_[rawStream_].dequeue());
> > +			captureRaw_ = false;
> > +		} else {
> > +			qWarning() << "No free buffer available for RAW capture";
> > +		}
> > +	}
> 
> We can minimize the amount of code covered by the lock.
> 
> 	if (captureRaw_) {
> 		FrameBuffer *buffer;
> 
> 		{
> 			QMutexLocker locker(&mutex_);
> 			buffer = !freeBuffers_[rawStream_].isEmpty())
> 			       ? freeBuffers_[rawStream_].dequeue() : nullptr;
> 		}
> 
> 		if (buffer) {
> 			request->addBuffer(rawStream_, buffer);
> 			captureRaw_ = false;
> 		} else {
> 			qWarning() << "No free buffer available for RAW capture";
> 		}
> 	}

Good idea, thanks.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> >  	camera_->queueRequest(request);
> >  }
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 4856ecc10729159c..295ecc537e9d45bf 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -55,6 +55,8 @@ private Q_SLOTS:
> >  	void toggleCapture(bool start);
> >  
> >  	void saveImageAs();
> > +	void captureRaw();
> > +	void processRaw(FrameBuffer *buffer);
> >  
> >  	void queueRequest(FrameBuffer *buffer);
> >  
> > @@ -75,6 +77,7 @@ private:
> >  	QToolBar *toolbar_;
> >  	QAction *startStopAction_;
> >  	QComboBox *cameraCombo_;
> > +	QAction *saveRaw_;
> >  	ViewFinder *viewfinder_;
> >  
> >  	QIcon iconPlay_;
> > @@ -96,6 +99,7 @@ private:
> >  
> >  	/* Capture state, buffers queue and statistics */
> >  	bool isCapturing_;
> > +	bool captureRaw_;
> >  	Stream *vfStream_;
> >  	Stream *rawStream_;
> >  	std::map<Stream *, QQueue<FrameBuffer *>> freeBuffers_;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc
index c4eb7a0be6884373..fc8213928ece70ea 100644
--- a/src/qcam/assets/feathericons/feathericons.qrc
+++ b/src/qcam/assets/feathericons/feathericons.qrc
@@ -1,5 +1,6 @@ 
 <!DOCTYPE RCC><RCC version="1.0">
 <qresource>
+<file>./aperture.svg</file>
 <file>./camera-off.svg</file>
 <file>./play-circle.svg</file>
 <file>./save.svg</file>
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
index 0bd9f3583ea4f6d4..458da479a9b21b73 100644
--- a/src/qcam/main_window.cpp
+++ b/src/qcam/main_window.cpp
@@ -27,6 +27,10 @@ 
 #include <libcamera/camera_manager.h>
 #include <libcamera/version.h>
 
+#if HAVE_TIFF
+#include "dng_writer.h"
+#endif
+
 using namespace libcamera;
 
 /**
@@ -48,7 +52,8 @@  public:
 };
 
 MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
-	: options_(options), cm_(cm), allocator_(nullptr), isCapturing_(false)
+	: saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr),
+	  isCapturing_(false), captureRaw_(false)
 {
 	int ret;
 
@@ -144,6 +149,16 @@  int MainWindow::createToolbars()
 	action->setShortcut(QKeySequence::SaveAs);
 	connect(action, &QAction::triggered, this, &MainWindow::saveImageAs);
 
+#if HAVE_TIFF
+	/* Save Raw action. */
+	action = toolbar_->addAction(QIcon::fromTheme("camera-photo",
+						      QIcon(":aperture.svg")),
+				     "Save Raw");
+	action->setEnabled(false);
+	connect(action, &QAction::triggered, this, &MainWindow::captureRaw);
+	saveRaw_ = action;
+#endif
+
 	return 0;
 }
 
@@ -369,6 +384,10 @@  int MainWindow::startCapture()
 
 	adjustSize();
 
+	/* Configure the raw capture button. */
+	if (saveRaw_)
+		saveRaw_->setEnabled(config_->size() == 2);
+
 	/* Allocate and map buffers. */
 	allocator_ = new FrameBufferAllocator(camera_);
 	for (StreamConfiguration &config : *config_) {
@@ -474,6 +493,9 @@  void MainWindow::stopCapture()
 		return;
 
 	viewfinder_->stop();
+	if (saveRaw_)
+		saveRaw_->setEnabled(false);
+	captureRaw_ = false;
 
 	int ret = camera_->stop();
 	if (ret)
@@ -524,6 +546,31 @@  void MainWindow::saveImageAs()
 	writer.write(image);
 }
 
+void MainWindow::captureRaw()
+{
+	captureRaw_ = true;
+}
+
+void MainWindow::processRaw(FrameBuffer *buffer)
+{
+#if HAVE_TIFF
+	QString defaultPath = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation);
+	QString filename = QFileDialog::getSaveFileName(this, "Save DNG", defaultPath,
+							"DNG Files (*.dng)");
+
+	if (filename != "") {
+		const MappedBuffer &mapped = mappedBuffers_[buffer];
+		DNGWriter::write(filename.toStdString().c_str(), camera_.get(),
+				 rawStream_->configuration(), buffer,
+				 mapped.memory);
+	}
+#endif
+	{
+		QMutexLocker locker(&mutex_);
+		freeBuffers_[rawStream_].enqueue(buffer);
+	}
+}
+
 /* -----------------------------------------------------------------------------
  * Request Completion Handling
  */
@@ -566,6 +613,9 @@  void MainWindow::processCapture()
 	/* Process buffers. */
 	if (buffers.count(vfStream_))
 		processViewfinder(buffers[vfStream_]);
+
+	if (buffers.count(rawStream_))
+		processRaw(buffers[rawStream_]);
 }
 
 void MainWindow::processViewfinder(FrameBuffer *buffer)
@@ -598,5 +648,17 @@  void MainWindow::queueRequest(FrameBuffer *buffer)
 
 	request->addBuffer(vfStream_, buffer);
 
+	if (captureRaw_) {
+		QMutexLocker locker(&mutex_);
+
+		if (!freeBuffers_[rawStream_].isEmpty()) {
+			request->addBuffer(rawStream_,
+					   freeBuffers_[rawStream_].dequeue());
+			captureRaw_ = false;
+		} else {
+			qWarning() << "No free buffer available for RAW capture";
+		}
+	}
+
 	camera_->queueRequest(request);
 }
diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
index 4856ecc10729159c..295ecc537e9d45bf 100644
--- a/src/qcam/main_window.h
+++ b/src/qcam/main_window.h
@@ -55,6 +55,8 @@  private Q_SLOTS:
 	void toggleCapture(bool start);
 
 	void saveImageAs();
+	void captureRaw();
+	void processRaw(FrameBuffer *buffer);
 
 	void queueRequest(FrameBuffer *buffer);
 
@@ -75,6 +77,7 @@  private:
 	QToolBar *toolbar_;
 	QAction *startStopAction_;
 	QComboBox *cameraCombo_;
+	QAction *saveRaw_;
 	ViewFinder *viewfinder_;
 
 	QIcon iconPlay_;
@@ -96,6 +99,7 @@  private:
 
 	/* Capture state, buffers queue and statistics */
 	bool isCapturing_;
+	bool captureRaw_;
 	Stream *vfStream_;
 	Stream *rawStream_;
 	std::map<Stream *, QQueue<FrameBuffer *>> freeBuffers_;