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

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

Commit Message

Niklas Söderlund May 1, 2020, 3:27 p.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>
---
 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(-)

Comments

Laurent Pinchart May 1, 2020, 5:33 p.m. UTC | #1 
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@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_;
Niklas Söderlund May 2, 2020, 1:59 a.m. UTC | #2 
Hi Laurent,

Thanks for your feedback.

On 2020-05-01 20:33:38 +0300, Laurent Pinchart wrote:
> 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@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 ?

If you did I missed it, sorry about that. But I agree it should be set 
to false here.

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

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