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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2020-03-23 13:12:31 +0200, Laurent Pinchart wrote:
> 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) ?

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.

> 
> > +
> > +	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

-- 
Regards,
Niklas Söderlund