[libcamera-devel] [RFC PATCH v2 04/10] cam: file_sink: Use offset in mapping FrameBuffer

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Aug 23, 2021 at 10:12:15PM +0900, Hirokazu Honda wrote:
> This fixes the way of mapping FrameBuffer in FrameSink by
> using offset.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/cam/file_sink.cpp | 22 +++++++++++++++++-----
>  src/cam/file_sink.h   |  1 +
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp
> index 2d30694a..26a60058 100644
> --- a/src/cam/file_sink.cpp
> +++ b/src/cam/file_sink.cpp
> @@ -51,12 +51,24 @@ int FileSink::configure(const libcamera::CameraConfiguration &config)
>  
>  void FileSink::mapBuffer(FrameBuffer *buffer)
>  {
> +	/* \todo use MappedFrameBuffer. */
>  	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> -		void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> -				    plane.fd.fd(), 0);
> +		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 ?
> +                         */

Tabs for indentation.

> +			size_t length = lseek(fd, 0, SEEK_END);
> +			void *memory = mmap(NULL, plane.length, PROT_READ,
> +					    MAP_SHARED, plane.fd.fd(), 0);
> +			mappedBuffers_[plane.fd.fd()] =
> +				std::make_pair(memory, length);

You can use fd instead of plane.fd.fd().

>  

And you can drop this blank line.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> -		mappedBuffers_[plane.fd.fd()] =
> -			std::make_pair(memory, plane.length);
> +		}
> +
> +		void *memory = mappedBuffers_[fd].first;
> +		planeData_[&plane] = static_cast<uint8_t *>(memory) + plane.offset;
>  	}
>  }
>  
> @@ -102,7 +114,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)
>  		const FrameBuffer::Plane &plane = buffer->planes()[i];
>  		const FrameMetadata::Plane &meta = buffer->metadata().planes[i];
>  
> -		void *data = mappedBuffers_[plane.fd.fd()].first;
> +		uint8_t *data = planeData_[&plane];
>  		unsigned int length = std::min(meta.bytesused, plane.length);
>  
>  		if (meta.bytesused > plane.length)
> diff --git a/src/cam/file_sink.h b/src/cam/file_sink.h
> index c3eb230a..c12325d9 100644
> --- a/src/cam/file_sink.h
> +++ b/src/cam/file_sink.h
> @@ -33,6 +33,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_;
>  };
>  
>  #endif /* __CAM_FILE_SINK_H__ */

-- 
Regards,

Laurent Pinchart