[libcamera-devel] [PATCH 05/21] qcam: main_window: Move capture event processing to main thread

[Kieran Bingham]

Hi Laurent,

On 23/03/2020 16:52, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Mar 23, 2020 at 02:59:08PM +0000, Kieran Bingham wrote:
>> On 23/03/2020 14:21, Laurent Pinchart wrote:
>>> To avoid blocking the camera manager for a long amount of time, move
>>> capture event processing to the main thread. Captured buffers are added
>>> to a queue and an event is posted to the main window to signal
>>> availability of a buffer.
>>
>> Is the 'queue' necessary?
>> Could the buffers not be passed through the event?
> 
> I would then have no way in processCapture() to ignore the event after
> stopping the camera. I could check isCapturing_, but at least in theory,
> we could stop and restart and have the pending event generated before
> stopped delivered after the restart. I think a queue is cleaner as it
> gives us full control.
> 

Ok, with minors resolved,

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>>  src/qcam/main_window.cpp | 54 +++++++++++++++++++++++++++++++++++++++-
>>>  src/qcam/main_window.h   |  8 ++++++
>>>  2 files changed, 61 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
>>> index 354a53367d0f..afd8b9c413f5 100644
>>> --- a/src/qcam/main_window.cpp
>>> +++ b/src/qcam/main_window.cpp
>>> @@ -19,6 +19,7 @@
>>>  #include <QImage>
>>>  #include <QImageWriter>
>>>  #include <QInputDialog>
>>> +#include <QMutexLocker>
>>>  #include <QStandardPaths>
>>>  #include <QTimer>
>>>  #include <QToolBar>
>>> @@ -31,6 +32,21 @@
>>>  
>>>  using namespace libcamera;
>>>  
>>> +class CaptureEvent : public QEvent
>>> +{
>>> +public:
>>> +	CaptureEvent()
>>> +		: QEvent(type())
>>> +	{
>>> +	}
>>> +
>>> +	static Type type()
>>> +	{
>>> +		static int type = QEvent::registerEventType();
>>> +		return static_cast<Type>(type);
>>> +	}
>>> +};
>>> +
>>>  MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options)
>>>  	: options_(options), cm_(cm), allocator_(nullptr), isCapturing_(false)
>>>  {
>>> @@ -63,6 +79,16 @@ MainWindow::~MainWindow()
>>>  	}
>>>  }
>>>  
>>> +bool MainWindow::event(QEvent *e)
>>> +{
>>> +	if (e->type() == CaptureEvent::type()) {
>>> +		processCapture();
>>> +		return true;
>>> +	}
>>> +
>>> +	return QMainWindow::event(e);
>>> +}
>>> +
>>>  int MainWindow::createToolbars()
>>>  {
>>>  	QAction *action;
>>> @@ -343,6 +369,13 @@ void MainWindow::stopCapture()
>>>  
>>>  	config_.reset();
>>>  
>>> +	/*
>>> +	 * A CaptureEvent may have been posted before we stopped the camera,
>>> +	 * but not processed yet. Clear the queue of done buffers to avoid
>>> +	 * racing with the event handler.
>>> +	 */
>>> +	doneQueue_.clear();
>>> +
>>
>> Do we not need to lock mutex_ here?
> 
> No, because at this point there can't be any call to
> MainWindow::requestComplete(), as the camera is stopped. This function,
> as well as MainWindow::processCapture(), run in the same thread, so they
> can't run concurrently.
> 
>>>  	titleTimer_.stop();
>>>  	setWindowTitle(title_);
>>>  }
>>> @@ -371,10 +404,29 @@ void MainWindow::requestComplete(Request *request)
>>>  		return;
>>>  
>>>  	const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
>>> +	FrameBuffer *buffer = buffers.begin()->second;
>>> +
>>> +	{
>>> +		QMutexLocker locker(&mutex_);
>>> +		doneQueue_.enqueue(buffer);
>>> +	}
>>> +
>>> +	QCoreApplication::postEvent(this, new CaptureEvent);
>>> +}
>>> +
>>> +void MainWindow::processCapture()
>>> +{
>>> +	FrameBuffer *buffer;
>>
>> This scoping is to ensure the locker unlocks after the dequeue right?
> 
> Correct.
> 
>> Perhaps a new line between the FrameBuffer declaration and the start of
>> the scope? It looks a bit like some sort of badly defined inline
>> function otherwise :-S
> 
> OK.
> 
>>> +	{
>>> +		QMutexLocker locker(&mutex_);
>>> +		if (doneQueue_.isEmpty())
>>> +			return;
>>> +
>>> +		buffer = doneQueue_.dequeue();
>>> +	}
>>>  
>>>  	framesCaptured_++;
>>>  
>>> -	FrameBuffer *buffer = buffers.begin()->second;
>>>  	const FrameMetadata &metadata = buffer->metadata();
>>>  
>>>  	double fps = metadata.timestamp - lastBufferTime_;
>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
>>> index 720a3393e3dc..c623120d5894 100644
>>> --- a/src/qcam/main_window.h
>>> +++ b/src/qcam/main_window.h
>>> @@ -11,7 +11,9 @@
>>>  
>>>  #include <QElapsedTimer>
>>>  #include <QMainWindow>
>>> +#include <QMutex>
>>>  #include <QObject>
>>> +#include <QQueue>
>>>  #include <QTimer>
>>>  
>>>  #include <libcamera/buffer.h>
>>> @@ -40,6 +42,8 @@ public:
>>>  	MainWindow(CameraManager *cm, const OptionsParser::Options &options);
>>>  	~MainWindow();
>>>  
>>> +	bool event(QEvent *e) override;
>>> +
>>>  private Q_SLOTS:
>>>  	void quit();
>>>  	void updateTitle();
>>> @@ -57,6 +61,7 @@ private:
>>>  	int openCamera();
>>>  
>>>  	void requestComplete(Request *request);
>>> +	void processCapture();
>>>  	int display(FrameBuffer *buffer);
>>>  	void queueRequest(FrameBuffer *buffer);
>>>  
>>> @@ -78,6 +83,9 @@ private:
>>>  	uint32_t previousFrames_;
>>>  	uint32_t framesCaptured_;
>>>  
>>> +	QMutex mutex_;
>>> +	QQueue<FrameBuffer *> doneQueue_;
>>> +
>>>  	QToolBar *toolbar_;
>>>  	ViewFinder *viewfinder_;
>>>  	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
> 

-- 
Regards
--
Kieran