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

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Aug 16, 2021 at 01:31:32PM +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 | 5 ++++-
>  src/cam/file_sink.h   | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp
> index 2d30694a..92b2d53d 100644
> --- a/src/cam/file_sink.cpp
> +++ b/src/cam/file_sink.cpp
> @@ -51,12 +51,15 @@ 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);
>  
>  		mappedBuffers_[plane.fd.fd()] =
>  			std::make_pair(memory, plane.length);
> +		planeData_[plane.fd.fd()] =
> +			static_cast<unsigned int *>(memory) + plane.offset;

I don't think this will work. With an NV12 buffer, for instance,
plane.fd.fd() will be the same for both planes, so you'll overwrite
planeData_ of the first plane with the second plane. It's fine in this
patch, as we don't create FrameBuffer instances with multiple planes,
but it will break as soon as we do. Should we instead store the
addresses in a std::map<std::pair<Buffer *, unsigned int>, void *> (with
the unsigned int being the plane index) ? Of maybe just a
std::map<FrameBuffer::Plane *, void *> ?

>  	}
>  }
>  
> @@ -102,7 +105,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;
> +		void *data = planeData_[plane.fd.fd()];
>  		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..d236d6d8 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<int, void *> planeData_;
>  };
>  
>  #endif /* __CAM_FILE_SINK_H__ */

-- 
Regards,

Laurent Pinchart