[libcamera-devel] [RFC PATCH 3/8] qcam: Convert to use MappedFrameBuffer

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Mon, Jul 20, 2020 at 11:42:27PM +0100, Kieran Bingham wrote:
> Remove the local mapping code, and utilise the libcamera buffer map
> helper class.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> 
> Working towards a more generic MappedBuffer base interface of the newly
> introduced MappedFrameBuffer hits a snag that the viewfinder in qcam
> already defines a type which conflicts.
> 
> Remove it by converting qcam to use the new MappedFrameBuffer types.

I'd drop this as I think we should start with MappedFrameBuffer being an
internal class.

>  src/qcam/main_window.cpp | 27 +++++++++++----------------
>  src/qcam/main_window.h   |  2 +-
>  src/qcam/viewfinder.cpp  |  4 ++--
>  src/qcam/viewfinder.h    |  7 +------
>  4 files changed, 15 insertions(+), 25 deletions(-)
> 
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 7bc13603774e..b25260ba7b28 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -446,10 +446,14 @@ int MainWindow::startCapture()
>  
>  		for (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {
>  			/* Map memory buffers and cache the mappings. */
> -			const FrameBuffer::Plane &plane = buffer->planes().front();
> -			void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> -					    plane.fd.fd(), 0);
> -			mappedBuffers_[buffer.get()] = { memory, plane.length };
> +			MappedFrameBuffer mapped(buffer.get(), PROT_READ);
> +			if (!mapped.isValid()) {
> +				qWarning() << "Failed to map buffer";
> +				ret = mapped.error();
> +				goto error;
> +			}
> +
> +			mappedBuffers_.insert({buffer.get(), std::move(mapped)});
>  
>  			/* Store buffers on the free list. */
>  			freeBuffers_[stream].enqueue(buffer.get());
> @@ -512,12 +516,7 @@ error:
>  	for (Request *request : requests)
>  		delete request;
>  
> -	for (auto &iter : mappedBuffers_) {
> -		const MappedBuffer &buffer = iter.second;
> -		munmap(buffer.memory, buffer.size);
> -	}
>  	mappedBuffers_.clear();
> -
>  	freeBuffers_.clear();
>  
>  	delete allocator_;
> @@ -548,10 +547,6 @@ void MainWindow::stopCapture()
>  
>  	camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);
>  
> -	for (auto &iter : mappedBuffers_) {
> -		const MappedBuffer &buffer = iter.second;
> -		munmap(buffer.memory, buffer.size);
> -	}
>  	mappedBuffers_.clear();
>  
>  	delete allocator_;
> @@ -643,10 +638,10 @@ void MainWindow::processRaw(FrameBuffer *buffer, const ControlList &metadata)
>  							"DNG Files (*.dng)");
>  
>  	if (!filename.isEmpty()) {
> -		const MappedBuffer &mapped = mappedBuffers_[buffer];
> +		const MappedFrameBuffer &mapped = mappedBuffers_.at(buffer);
>  		DNGWriter::write(filename.toStdString().c_str(), camera_.get(),
>  				 rawStream_->configuration(), metadata, buffer,
> -				 mapped.memory);
> +				 mapped.maps()[0].address);
>  	}
>  #endif
>  
> @@ -720,7 +715,7 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
>  		<< "fps:" << Qt::fixed << qSetRealNumberPrecision(2) << fps;
>  
>  	/* Render the frame on the viewfinder. */
> -	viewfinder_->render(buffer, &mappedBuffers_[buffer]);
> +	viewfinder_->render(buffer, &mappedBuffers_.at(buffer));
>  }
>  
>  void MainWindow::queueRequest(FrameBuffer *buffer)
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 4606fe487ad4..ddf51ca01111 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -119,7 +119,7 @@ private:
>  	FrameBufferAllocator *allocator_;
>  
>  	std::unique_ptr<CameraConfiguration> config_;
> -	std::map<FrameBuffer *, MappedBuffer> mappedBuffers_;
> +	std::map<FrameBuffer *, MappedFrameBuffer> mappedBuffers_;
>  
>  	/* Capture state, buffers queue and statistics */
>  	bool isCapturing_;
> diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp
> index dcf8a15d2df6..591d26eae87c 100644
> --- a/src/qcam/viewfinder.cpp
> +++ b/src/qcam/viewfinder.cpp
> @@ -78,14 +78,14 @@ int ViewFinder::setFormat(const libcamera::PixelFormat &format,
>  	return 0;
>  }
>  
> -void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)
> +void ViewFinder::render(libcamera::FrameBuffer *buffer, libcamera::MappedFrameBuffer *map)
>  {
>  	if (buffer->planes().size() != 1) {
>  		qWarning() << "Multi-planar buffers are not supported";
>  		return;
>  	}
>  
> -	unsigned char *memory = static_cast<unsigned char *>(map->memory);
> +	unsigned char *memory = static_cast<unsigned char *>(map->maps()[0].address);
>  	size_t size = buffer->metadata().planes[0].bytesused;
>  
>  	{
> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h
> index 26a1320537d2..c4cc51f14dda 100644
> --- a/src/qcam/viewfinder.h
> +++ b/src/qcam/viewfinder.h
> @@ -23,11 +23,6 @@
>  
>  class QImage;
>  
> -struct MappedBuffer {
> -	void *memory;
> -	size_t size;
> -};
> -
>  class ViewFinder : public QWidget
>  {
>  	Q_OBJECT
> @@ -39,7 +34,7 @@ public:
>  	const QList<libcamera::PixelFormat> &nativeFormats() const;
>  
>  	int setFormat(const libcamera::PixelFormat &format, const QSize &size);
> -	void render(libcamera::FrameBuffer *buffer, MappedBuffer *map);
> +	void render(libcamera::FrameBuffer *buffer, libcamera::MappedFrameBuffer *map);
>  	void stop();
>  
>  	QImage getCurrentImage();

-- 
Regards,

Laurent Pinchart