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

[Niklas Söderlund]

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

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

-- 
Regards,
Niklas Söderlund