[libcamera-devel] [PATCH v3 3/3] qcam: Add RAW capture support

[Laurent Pinchart]

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 at 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 at 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