[libcamera-devel] [PATCH 4/7] libcamera: FrameBuffer: Add a method to copy buffer content

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tue, Mar 24, 2020 at 04:51:42PM +0100, Niklas Söderlund wrote:
> This method may be used to memory copy a whole FrameBuffer content to
> from another buffer. The operation is not fast and should not be used

to or from ? :-)

> without great care by pipelines.
> 
> The intended use-case is to have an option to copy out RAW buffers from
> the middle of a pipeline.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since RFC
> - Make method member of FrameBuffer
> - Add documentation
> - s/out/dstmem/ and s/in/src/mem/
> ---
>  include/libcamera/buffer.h |  1 +
>  src/libcamera/buffer.cpp   | 61 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 8e5ec699e3925eee..ef3a3b36cd4e4e17 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -57,6 +57,7 @@ public:
>  	unsigned int cookie() const { return cookie_; }
>  	void setCookie(unsigned int cookie) { cookie_ = cookie; }
>  
> +	int copyFrom(const FrameBuffer *src);
>  private:
>  	friend class Request; /* Needed to update request_. */
>  	friend class V4L2VideoDevice; /* Needed to update metadata_. */
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 673a63d3d1658190..0352917e9f2a3202 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -211,4 +211,65 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
>   * core never modifies the buffer cookie.
>   */
>  
> +/**
> + * \brief Copy the content from another buffer

I'm not entirely sure, but I think s/content/contents/ (same below).
Maybe Kieran can shed some native speaker light here.

> + * \param[in] src Buffer to copy
> + *
> + * Copy the buffer content and metadata form \a src. The receiving FrameBuffer

s/form/from/
s/src/src to this buffer/
s/receiving/destination/

> + * needs to have at least as many planes of equal or grater size then the source.

This doesn't match the implementation, should it be "shall have the same
number of planes as the source buffer, and each destination plane shall
be larger than or equal to the corresponding source plane." ?

Should we add a sentence to explain how metadata is copied ? This could
do, but feel free to rephrase it:

"The complete metadata of the source buffer is copied to the destination
buffer. If an error occurs during the copy, the destination buffer's
metadata status is set to FrameMetadata::FrameError, and other metadata
fields are not modified."

> + *
> + * The operation is performed using memcpy() so is very slow, users needs to
> + * consider this before copying buffers.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int FrameBuffer::copyFrom(const FrameBuffer *src)
> +{
> +	if (planes_.size() != src->planes_.size()) {
> +		LOG(Buffer, Error) << "Different number of planes";
> +		metadata_.status = FrameMetadata::FrameError;
> +		return -EINVAL;
> +	}
> +
> +	for (unsigned int i = 0; i < planes_.size(); i++) {
> +		if (planes_[i].length < src->planes_[i].length) {
> +			LOG(Buffer, Error) << "Plane " << i << " is too small";
> +			metadata_.status = FrameMetadata::FrameError;
> +			return -EINVAL;
> +		}
> +	}
> +
> +	for (unsigned int i = 0; i < planes_.size(); i++) {
> +		void *dstmem = mmap(NULL, planes_[i].length, PROT_WRITE,

s/NULL/nullptr/

> +				    MAP_SHARED, planes_[i].fd.fd(), 0);
> +
> +		if (dstmem == MAP_FAILED) {
> +			LOG(Buffer, Error)
> +				<< "Failed to map destination plane " << i;
> +			metadata_.status = FrameMetadata::FrameError;
> +			return -EINVAL;
> +		}
> +
> +		void *srcmem = mmap(NULL, src->planes_[i].length, PROT_READ,

s/NULL/nullptr/

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

> +				    MAP_SHARED, src->planes_[i].fd.fd(), 0);
> +
> +		if (srcmem == MAP_FAILED) {
> +			munmap(dstmem, planes_[i].length);
> +			LOG(Buffer, Error)
> +				<< "Failed to map source plane " << i;
> +			metadata_.status = FrameMetadata::FrameError;
> +			return -EINVAL;
> +		}
> +
> +		memcpy(dstmem, srcmem, src->planes_[i].length);
> +
> +		munmap(srcmem, src->planes_[i].length);
> +		munmap(dstmem, planes_[i].length);
> +	}
> +
> +	metadata_ = src->metadata_;
> +
> +	return 0;
> +}
> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart