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

[Kieran Bingham]

Hi Niklas,

On 22/09/2020 11:02, Niklas Söderlund wrote:
> 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 :-)

Aha I see ... I guess it depends on how the data gets exposed indeed,
and whether the 'raw' data is sent to applications, or if we translate
it in anyway to make it more generically consumable.

Initially I would assume there would be a need to translate to generic
formats, otherwise each application might have to be pipeline specific -
but then again - for camera tuning applications, it might *be* pipeline
specific ...

Dropping seems fine for me for now all the same, it's easy to bring back
if we hit that later.

--
Kieran


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