[libcamera-devel] [RFC 4/6] libcamera: buffer: Add helper to memcopy a FrameBuffer

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Mar 16, 2020 at 03:41:44AM +0100, Niklas Söderlund wrote:
> This helper may be used to memory copy a while FrameBuffer content to

s/while/whole/ ?

> another buffer. The operation is not fast and should not be used without
> grate care by pipelines.

Unless you really meant to talk about BBQs, s/grate/great/

> 
> The intended use-case is to have an option to copy out RAW buffers from
> the middle of an pipeline.

s/an/a/

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/buffer.h |  1 +
>  src/libcamera/buffer.cpp   | 43 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 8e5ec699e3925eee..669d2591a90e5f37 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -60,6 +60,7 @@ public:
>  private:
>  	friend class Request; /* Needed to update request_. */
>  	friend class V4L2VideoDevice; /* Needed to update metadata_. */
> +	friend int FrameBufferMemCopy(FrameBuffer *destination, const FrameBuffer *source);
>  
>  	std::vector<Plane> planes_;
>  
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 673a63d3d1658190..f680d1879b57a68b 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -211,4 +211,47 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
>   * core never modifies the buffer cookie.
>   */
>  
> +int FrameBufferMemCopy(FrameBuffer *dst, const FrameBuffer *src)

This is a function, it should start with a lower-case f, and should have
a proper declaration in buffer.h. This being said, I think it would be
best to move it to a member function. How about

int FrameBuffer::copyTo(FrameBuffer *dst)

? Or maybe FrameBuffer::copyFrom(FrameBuffer *src) ?

Documentation is also needed.

> +{
> +	if (dst->planes_.size() != src->planes_.size()) {
> +		LOG(Buffer, Error) << "Different number of planes";
> +		return -EINVAL;
> +	}
> +
> +	for (unsigned int i = 0; i < dst->planes_.size(); i++) {
> +		if (dst->planes_[i].length < src->planes_[i].length) {
> +			LOG(Buffer, Error) << "Plane " << i << " is too small";
> +			return -EINVAL;
> +		}
> +	}

We don't support that yet, so it's not really much of a concern, but
will we have to handle the case where the stride differs ? And how about
data offsets (when we'll have them too) ? Will we store that information
in FrameBuffer::Plane or FrameMetadata::Plane ? I suspect the latter, so
we'll have to ensure that metadata is valid, is it guaranteed ?

> +
> +	for (unsigned int i = 0; i < dst->planes_.size(); i++) {
> +		void *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE, MAP_SHARED,
> +			   dst->planes_[i].fd.fd(), 0);

		void *out = mmap(NULL, dst->planes_[i].length, PROT_WRITE,
				 MAP_SHARED, dst->planes_[i].fd.fd(), 0);

And I think I'd name the variable dstmem or something similar.

> +
> +		if (out == MAP_FAILED) {
> +			LOG(Buffer, Error) << "Failed to map output plane " << i;

s/output/destination/

> +			return -EINVAL;
> +		}
> +
> +		void *in = mmap(NULL, src->planes_[i].length, PROT_READ, MAP_SHARED,
> +			  src->planes_[i].fd.fd(), 0);

Same here, with srcmem.

> +
> +		if (in == MAP_FAILED) {
> +			munmap(out, dst->planes_[i].length);
> +			LOG(Buffer, Error) << "Failed to map input plane " << i;

s/input/source/

> +			return -EINVAL;
> +		}
> +
> +		memcpy(out, in, src->planes_[i].length);

length, or bytesused from the metadata ?

> +
> +		munmap(in, src->planes_[i].length);
> +		munmap(out, dst->planes_[i].length);

I really don't like how we create and tear down mappings every time :-(
The alternative is to cache the mappings in the FrameBuffer class, but
that's a slippery slope. We may not need to fix this now, but I think we
need to consider our options.

> +	}
> +
> +	dst->metadata_ = src->metadata_;
> +
> +	return 0;
> +}
> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart