[libcamera-devel] [PATCH v2 20/27] cam: file_sink: Use Image class to access pixel data

[Jean-Michel Hautbois]

Hi Laurent,

On 06/09/2021 04:00, Laurent Pinchart wrote:
> Replace the manual implementation of frame buffer mapping with the Image
> class to improve code sharing.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

It improves reading indeed !
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>

> ---
>  src/cam/file_sink.cpp | 42 +++++++++++++-----------------------------
>  src/cam/file_sink.h   |  6 ++++--
>  2 files changed, 17 insertions(+), 31 deletions(-)
> 
> diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp
> index 0fc7d621f50b..3c2e565b27a2 100644
> --- a/src/cam/file_sink.cpp
> +++ b/src/cam/file_sink.cpp
> @@ -5,17 +5,18 @@
>   * file_sink.cpp - File Sink
>   */
>  
> +#include <assert.h>
>  #include <fcntl.h>
>  #include <iomanip>
>  #include <iostream>
>  #include <sstream>
>  #include <string.h>
> -#include <sys/mman.h>
>  #include <unistd.h>
>  
>  #include <libcamera/camera.h>
>  
>  #include "file_sink.h"
> +#include "image.h"
>  
>  using namespace libcamera;
>  
> @@ -26,12 +27,6 @@ FileSink::FileSink(const std::string &pattern)
>  
>  FileSink::~FileSink()
>  {
> -	for (auto &iter : mappedBuffers_) {
> -		void *memory = iter.second.first;
> -		unsigned int length = iter.second.second;
> -		munmap(memory, length);
> -	}
> -	mappedBuffers_.clear();
>  }
>  
>  int FileSink::configure(const libcamera::CameraConfiguration &config)
> @@ -51,23 +46,11 @@ int FileSink::configure(const libcamera::CameraConfiguration &config)
>  
>  void FileSink::mapBuffer(FrameBuffer *buffer)
>  {
> -	/* \todo use MappedFrameBuffer. */
> -	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> -		const int fd = plane.fd.fd();
> -		if (mappedBuffers_.find(fd) == mappedBuffers_.end()) {
> -			/**
> -			 * \todo Should we try to only map the portions of the
> -			 * dmabuf that are used by planes ?
> -			 */
> -			size_t length = lseek(fd, 0, SEEK_END);
> -			void *memory = mmap(NULL, plane.length, PROT_READ,
> -					    MAP_SHARED, fd, 0);
> -			mappedBuffers_[fd] = std::make_pair(memory, length);
> -		}
> +	std::unique_ptr<Image> image =
> +		Image::fromFrameBuffer(buffer, Image::MapMode::ReadOnly);
> +	assert(image != nullptr);
>  
> -		void *memory = mappedBuffers_[fd].first;
> -		planeData_[&plane] = static_cast<uint8_t *>(memory) + plane.offset;
> -	}
> +	mappedBuffers_[buffer] = std::move(image);
>  }
>  
>  bool FileSink::processRequest(Request *request)
> @@ -108,19 +91,20 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)
>  		return;
>  	}
>  
> +	Image *image = mappedBuffers_[buffer].get();
> +
>  	for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> -		const FrameBuffer::Plane &plane = buffer->planes()[i];
>  		const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
>  
> -		uint8_t *data = planeData_[&plane];
> -		unsigned int length = std::min(meta.bytesused, plane.length);
> +		Span<uint8_t> data = image->data(i);
> +		unsigned int length = std::min<unsigned int>(meta.bytesused, data.size());
>  
> -		if (meta.bytesused > plane.length)
> +		if (meta.bytesused > data.size())
>  			std::cerr << "payload size " << meta.bytesused
> -				  << " larger than plane size " << plane.length
> +				  << " larger than plane size " << data.size()
>  				  << std::endl;
>  
> -		ret = ::write(fd, data, length);
> +		ret = ::write(fd, data.data(), length);
>  		if (ret < 0) {
>  			ret = -errno;
>  			std::cerr << "write error: " << strerror(-ret)
> diff --git a/src/cam/file_sink.h b/src/cam/file_sink.h
> index c12325d955c5..335be93b8732 100644
> --- a/src/cam/file_sink.h
> +++ b/src/cam/file_sink.h
> @@ -8,12 +8,15 @@
>  #define __CAM_FILE_SINK_H__
>  
>  #include <map>
> +#include <memory>
>  #include <string>
>  
>  #include <libcamera/stream.h>
>  
>  #include "frame_sink.h"
>  
> +class Image;
> +
>  class FileSink : public FrameSink
>  {
>  public:
> @@ -32,8 +35,7 @@ private:
>  
>  	std::map<const libcamera::Stream *, std::string> streamNames_;
>  	std::string pattern_;
> -	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
> -	std::map<const libcamera::FrameBuffer::Plane *, uint8_t *> planeData_;
> +	std::map<libcamera::FrameBuffer *, std::unique_ptr<Image>> mappedBuffers_;
>  };
>  
>  #endif /* __CAM_FILE_SINK_H__ */
>