[libcamera-devel] [RFC] libcamera: buffer: Remove copyFrom()

[Niklas Söderlund]

Hello,

On 2020-09-21 21:33:02 +0100, Kieran Bingham wrote:
> On 21/09/2020 20:16, Laurent Pinchart wrote:
> > Hi Niklas,
> > 
> > Thank you for the patch.
> > 
> > On Mon, Sep 21, 2020 at 08:32:05PM +0200, Niklas Söderlund wrote:
> >> There are no user left of the copyFrom() operation, remove it.
> >>
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >> ---
> >>  include/libcamera/buffer.h |  2 --
> >>  src/libcamera/buffer.cpp   | 59 --------------------------------------
> >>  2 files changed, 61 deletions(-)
> >> ---
> >>
> >> Hello,
> >>
> >> I'm not sure it's wise to leave this operation in the code while we have 
> >> no users. But I do fear we might need it the future, but as we don't use 
> >> or test it I'm sure we will have modified FrameBuffer enough to make 
> >> this incomplete at that that so I think it's best to remove it. What do 
> >> you guys think?
> > 
> > Drop it :-)
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > We can always bring code back if needed.
> > 
> 
> Agreed, No users, no tests, no requirement, and an undesired 'feature'.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Thanks then i will move ahead and drop this code.

> 
> What can you envisage in the future that might require bringing it back?

I'm primary thinking about exposing statistics/embeddedmetadata to 
applications by copying them. As we are not sure how to best do that I 
fear a copy could be involved, or not :-)

> 
> 
> 
> >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> >> index 6bb2e4f8558f03ac..a26c8927d37a812e 100644
> >> --- a/include/libcamera/buffer.h
> >> +++ b/include/libcamera/buffer.h
> >> @@ -57,8 +57,6 @@ 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 03f628e8b585a1f9..75b2693281a74cdc 100644
> >> --- a/src/libcamera/buffer.cpp
> >> +++ b/src/libcamera/buffer.cpp
> >> @@ -226,65 +226,6 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> >>   * core never modifies the buffer cookie.
> >>   */
> >>  
> >> -/**
> >> - * \brief Copy the contents from another buffer
> >> - * \param[in] src FrameBuffer to copy
> >> - *
> >> - * Copy the buffer contents and metadata from \a src to this buffer. The
> >> - * destination FrameBuffer 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.
> >> - *
> >> - * 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;
> >> -		}
> >> -	}
> >> -
> >> -	MappedFrameBuffer source(src, PROT_READ);
> >> -	MappedFrameBuffer destination(this, PROT_WRITE);
> >> -
> >> -	if (!source.isValid()) {
> >> -		LOG(Buffer, Error) << "Failed to map source planes";
> >> -		return -EINVAL;
> >> -	}
> >> -
> >> -	if (!destination.isValid()) {
> >> -		LOG(Buffer, Error) << "Failed to map destination planes";
> >> -		return -EINVAL;
> >> -	}
> >> -
> >> -	for (unsigned int i = 0; i < planes_.size(); i++) {
> >> -		memcpy(destination.maps()[i].data(),
> >> -		       source.maps()[i].data(),
> >> -		       source.maps()[i].size());
> >> -	}
> >> -
> >> -	metadata_ = src->metadata_;
> >> -
> >> -	return 0;
> >> -}
> >> -
> >>  /**
> >>   * \class MappedBuffer
> >>   * \brief Provide an interface to support managing memory mapped buffers
> > 
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund