[libcamera-devel] [PATCH 13/21] qcam: viewfinder: Add MappedBuffer to store memory mapping information

[Kieran Bingham]

Hi Laurent,

On 23/03/2020 14:21, Laurent Pinchart wrote:
> The new MappedBuffer structure replaces the std::pair<> used in the
> mapped buffers map, and allows passing data to the ViewFinder::display()
> function in a more structured way.
> 

Seems reasonable for now ...

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

> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/qcam/main_window.cpp | 20 ++++++--------------
>  src/qcam/main_window.h   |  4 ++--
>  src/qcam/viewfinder.cpp  |  6 ++++--
>  src/qcam/viewfinder.h    | 10 +++++++++-
>  4 files changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 184345b0e2a6..c0feb91a6fb1 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -28,8 +28,6 @@
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/version.h>
>  
> -#include "viewfinder.h"
> -
>  using namespace libcamera;
>  
>  /**
> @@ -350,8 +348,7 @@ int MainWindow::startCapture()
>  		const FrameBuffer::Plane &plane = buffer->planes().front();
>  		void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
>  				    plane.fd.fd(), 0);
> -		mappedBuffers_[plane.fd.fd()] =
> -			std::make_pair(memory, plane.length);
> +		mappedBuffers_[buffer.get()] = { memory, plane.length };
>  	}
>  
>  	/* Start the title timer and the camera. */
> @@ -391,9 +388,8 @@ error:
>  		delete request;
>  
>  	for (auto &iter : mappedBuffers_) {
> -		void *memory = iter.second.first;
> -		unsigned int length = iter.second.second;
> -		munmap(memory, length);
> +		const MappedBuffer &buffer = iter.second;
> +		munmap(buffer.memory, buffer.size);
>  	}
>  	mappedBuffers_.clear();
>  
> @@ -421,9 +417,8 @@ void MainWindow::stopCapture()
>  	camera_->requestCompleted.disconnect(this, &MainWindow::requestComplete);
>  
>  	for (auto &iter : mappedBuffers_) {
> -		void *memory = iter.second.first;
> -		unsigned int length = iter.second.second;
> -		munmap(memory, length);
> +		const MappedBuffer &buffer = iter.second;
> +		munmap(buffer.memory, buffer.size);
>  	}
>  	mappedBuffers_.clear();
>  
> @@ -529,10 +524,7 @@ int MainWindow::display(FrameBuffer *buffer)
>  	if (buffer->planes().size() != 1)
>  		return -EINVAL;
>  
> -	const FrameBuffer::Plane &plane = buffer->planes().front();
> -	void *memory = mappedBuffers_[plane.fd.fd()].first;
> -	unsigned char *raw = static_cast<unsigned char *>(memory);
> -	viewfinder_->display(raw, buffer->metadata().planes[0].bytesused);
> +	viewfinder_->display(buffer, &mappedBuffers_[buffer]);

Wow, yes that's simpler!

>  
>  	return 0;
>  }
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index fcde88c14f77..0918ae5307d8 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -23,11 +23,11 @@
>  #include <libcamera/stream.h>
>  
>  #include "../cam/options.h"
> +#include "viewfinder.h"
>  
>  using namespace libcamera;
>  
>  class QAction;
> -class ViewFinder;
>  
>  enum {
>  	OptCamera = 'c',
> @@ -85,7 +85,7 @@ private:
>  	FrameBufferAllocator *allocator_;
>  
>  	std::unique_ptr<CameraConfiguration> config_;
> -	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
> +	std::map<FrameBuffer *, MappedBuffer> mappedBuffers_;
>  
>  	/* Capture state, buffers queue and statistics */
>  	bool isCapturing_;
> diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp
> index 066ac605e7b4..d00edc33dfb7 100644
> --- a/src/qcam/viewfinder.cpp
> +++ b/src/qcam/viewfinder.cpp
> @@ -24,7 +24,8 @@ ViewFinder::~ViewFinder()
>  	delete image_;
>  }
>  
> -void ViewFinder::display(const unsigned char *raw, size_t size)
> +void ViewFinder::display(const libcamera::FrameBuffer *buffer,
> +			 MappedBuffer *map)
>  {
>  	QMutexLocker locker(&mutex_);
>  
> @@ -34,7 +35,8 @@ void ViewFinder::display(const unsigned char *raw, size_t size)
>  	 * impacting performances.
>  	 */
>  
> -	converter_.convert(raw, size, image_);
> +	converter_.convert(static_cast<unsigned char *>(map->memory),
> +			   buffer->metadata().planes[0].bytesused, image_);
>  	update();
>  }
>  
> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h
> index a019c3a470ea..735a6b67e91d 100644
> --- a/src/qcam/viewfinder.h
> +++ b/src/qcam/viewfinder.h
> @@ -7,16 +7,24 @@
>  #ifndef __QCAM_VIEWFINDER_H__
>  #define __QCAM_VIEWFINDER_H__
>  
> +#include <stddef.h>
> +
>  #include <QMutex>
>  #include <QSize>
>  #include <QWidget>
>  
> +#include <libcamera/buffer.h>
>  #include <libcamera/pixelformats.h>
>  
>  #include "format_converter.h"
>  
>  class QImage;
>  
> +struct MappedBuffer {
> +	void *memory;
> +	size_t size;
> +};
> +
>  class ViewFinder : public QWidget
>  {
>  public:
> @@ -24,7 +32,7 @@ public:
>  	~ViewFinder();
>  
>  	int setFormat(const libcamera::PixelFormat &format, const QSize &size);
> -	void display(const unsigned char *rgb, size_t size);
> +	void display(const libcamera::FrameBuffer *buffer, MappedBuffer *map);
>  
>  	QImage getCurrentImage();
>  
> 

-- 
Regards
--
Kieran