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

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, May 01, 2020 at 05:27:45PM +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>
> ---
>  src/qcam/assets/feathericons/feathericons.qrc |  1 +
>  src/qcam/main_window.cpp                      | 40 ++++++++++++++++++-
>  src/qcam/main_window.h                        |  6 +++
>  3 files changed, 46 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 dc8824dae4669a7e..1c9948b98a231e05 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -48,7 +48,8 @@ public:
>  };
>  
>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
> -	: options_(options), cm_(cm), allocator_(nullptr), isCapturing_(false)
> +	: options_(options), cm_(cm), allocator_(nullptr), isCapturing_(false),
> +	  captureRaw_(false)
>  {
>  	int ret;
>  
> @@ -144,6 +145,14 @@ int MainWindow::createToolbars()
>  	action->setShortcut(QKeySequence::SaveAs);
>  	connect(action, &QAction::triggered, this, &MainWindow::saveImageAs);
>  
> +	/* Save Raw action. */
> +	action = toolbar_->addAction(QIcon::fromTheme("camera-photo",
> +						      QIcon(":aperture.svg")),
> +				     "Save Raw");
> +	action->setEnabled(false);
> +	connect(action, &QAction::triggered, this, &MainWindow::saveRaw);
> +	saveRaw_ = action;
> +
>  	return 0;
>  }
>  
> @@ -369,6 +378,9 @@ int MainWindow::startCapture()
>  
>  	adjustSize();
>  
> +	/* Configure the raw capture button. */
> +	saveRaw_->setEnabled(config_->size() == 2);
> +
>  	/* Allocate and map buffers. */
>  	allocator_ = new FrameBufferAllocator(camera_);
>  	for (StreamConfiguration &config : *config_) {
> @@ -474,6 +486,7 @@ void MainWindow::stopCapture()
>  		return;
>  
>  	viewfinder_->stop();
> +	saveRaw_->setEnabled(false);

I think I mentioned in my previous review that captureRaw_ should be set
to false here. Did you disagree with that ?

>  
>  	int ret = camera_->stop();
>  	if (ret)
> @@ -524,6 +537,11 @@ void MainWindow::saveImageAs()
>  	writer.write(image);
>  }
>  
> +void MainWindow::saveRaw()
> +{
> +	captureRaw_ = true;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Request Completion Handling
>   */
> @@ -567,6 +585,9 @@ void MainWindow::processCapture()
>  	if (buffers.count(vfStream_))
>  		processViewfinder(buffers[vfStream_]);
>  
> +	if (buffers.count(rawStream_))
> +		processRaw(buffers[rawStream_]);
> +
>  	/*
>  	 * Return buffers so they can be reused. No processing involving
>  	 * a buffer can happen after they are returned to the free list.
> @@ -603,6 +624,13 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
>  	viewfinder_->render(buffer, &mappedBuffers_[buffer]);
>  }
>  
> +void MainWindow::processRaw(FrameBuffer *buffer)
> +{
> +	const MappedBuffer &mapped = mappedBuffers_[buffer];
> +
> +	dngWriter_.write(camera_.get(), rawStream_, buffer, mapped.memory);
> +}
> +
>  void MainWindow::queueRequest(FrameBuffer *buffer)
>  {
>  	Request *request = camera_->createRequest();
> @@ -613,5 +641,15 @@ 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";
		}

Otherwise this looks OK, but should be adapted to move filename
generation out of DNGWriter, and to make DNG optional (with appropriate
#ifdef HAVE_TIFF, or maybe s/HAVE_TIFF/HAVE_DNG/ ?).

We should also pop up a dialog box to pick the file name. This can be
done in MainWindow::saveRaw() after captureRaw_ I believe, as processing
of captureRaw_ happens in a separate thread (if I'm not mistaken), but
we then need to wait until the file name is available before writing the
file. This is more complex, and can be done on top of this patch.

> +	}
> +
>  	camera_->queueRequest(request);
>  }
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 4856ecc10729159c..068eb2e38277768e 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -24,6 +24,7 @@
>  #include <libcamera/stream.h>
>  
>  #include "../cam/stream_options.h"
> +#include "dng_writer.h"
>  #include "viewfinder.h"
>  
>  using namespace libcamera;
> @@ -55,6 +56,7 @@ private Q_SLOTS:
>  	void toggleCapture(bool start);
>  
>  	void saveImageAs();
> +	void saveRaw();
>  
>  	void queueRequest(FrameBuffer *buffer);
>  
> @@ -70,11 +72,13 @@ private:
>  	void requestComplete(Request *request);
>  	void processCapture();
>  	void processViewfinder(FrameBuffer *buffer);
> +	void processRaw(FrameBuffer *buffer);
>  
>  	/* UI elements */
>  	QToolBar *toolbar_;
>  	QAction *startStopAction_;
>  	QComboBox *cameraCombo_;
> +	QAction *saveRaw_;
>  	ViewFinder *viewfinder_;
>  
>  	QIcon iconPlay_;
> @@ -96,11 +100,13 @@ private:
>  
>  	/* Capture state, buffers queue and statistics */
>  	bool isCapturing_;
> +	bool captureRaw_;
>  	Stream *vfStream_;
>  	Stream *rawStream_;
>  	std::map<Stream *, QQueue<FrameBuffer *>> freeBuffers_;
>  	QQueue<std::map<Stream *, FrameBuffer *>> doneQueue_;
>  	QMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */
> +	DNGWriter dngWriter_;
>  
>  	uint64_t lastBufferTime_;
>  	QElapsedTimer frameRateInterval_;

-- 
Regards,

Laurent Pinchart