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

[Laurent Pinchart]

Hi Niklas,

On Mon, Mar 23, 2020 at 08:07:27PM +0100, Niklas Söderlund wrote:
> On 2020-03-23 13:12:31 +0200, Laurent Pinchart wrote:
> > 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) ?
> 
> I like copyFrom().
> 
> > 
> > Documentation is also needed.
> 
> Yes.
> 
> >> +{
> >> +	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 ?
> 
> I'm not sure how we will deal with strides and offsets when we add 
> support for it. When I write this code my intention was to allow copy it 
> one-to-one. I thought about verifying the buffer status before allowing 
> it to be copied but decided against it.
> 
> Do you think such a check is the right thing? I don't feel strongly 
> about it.

If the source and destination have different strides then we should do a
stride-aware copy. Imagine a source with a padding to 128 bytes on every
line, and a destination that wouldn't have that constraint. The
destination size would actually be smaller than the source. We'd have to
copy the data, not the padding.

I wouldn't necessarily validate the state (the caller should know
better), but I think the stride will need to be taken into account as
it defines the data layout.

> >> +
> >> +	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 ?
> 
> I picked length as it describes the full buffer length. But it might 
> make sens to use bytesused as a small optimization. I think it goes hand 
> in hand with your question above, if the buffer status shall be 
> validated or not.
> 
> >> +
> >> +		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.
> 
> I don't like it either, but adding a cache of it to FrameBuffer even 
> less so. But maybe we will be forced to do so at some point.
> 
> >> +	}
> >> +
> >> +	dst->metadata_ = src->metadata_;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart