[libcamera-devel] [RFC 07/12] libcamera: buffer: Add dedicated container for buffer information

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Oct 28, 2019 at 03:25:20AM +0100, Niklas Söderlund wrote:
> The Buffer object will be split in two, one containing the memory and
> one containing the information recorded from when the buffer is
> dequeued. Add a container for the later in preparation for the split.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/buffer.h | 34 ++++++++++++++++++++++++++++++++++

This is missing documentation (hence the RFC status I suppose). I assume
you're aware of it :-)

>  1 file changed, 34 insertions(+)
> 
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 54c757ef7db8b5f6..c626f669040b3c04 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -105,6 +105,40 @@ private:
>  	Stream *stream_;
>  };
>  
> +class BufferInfo

An alternative name could be BufferMetaData. I wanted to mention it as
that's what I envisioned when we discussed this topic a few weeks ago.
We need a really good naming scheme for the buffer-related API, and
BufferInfo may be best, let's see how it fits in the big picture.

> +{
> +public:
> +	enum Status {
> +		BufferSuccess,
> +		BufferError,
> +		BufferCancelled,
> +	};
> +
> +	struct PlaneInfo {
> +		unsigned int bytesused;
> +	};

Do you foresee more plane-specific fields in the future, or should be
drop this structures and replace planes_ with a vector of bytesused ? I
think I'm leaning a bit towards keeping it as-is, but I may change my
mind while reviewing the rest of the series.

> +
> +	BufferInfo(Status status, unsigned int sequence, uint64_t timestamp,
> +		   const std::vector<PlaneInfo> &planes)
> +		: status_(status), sequence_(sequence), timestamp_(timestamp),
> +		  planes_(planes)
> +	{
> +	}
> +
> +	Status status() const { return status_; }
> +	unsigned int sequence() const { return sequence_; }
> +	unsigned int timestamp() const { return timestamp_; }

The return type doesn't match the timestamp_ member type.

> +
> +	unsigned int planes() const { return planes_.size(); }
> +	const PlaneInfo &plane(unsigned int plane) const { return planes_.at(plane); }

How about replacing those two methods with

	const std::vector<PlaneInfo> &planes() const { return planes_; }

? The planes() method above is a bit ill-named, as it returns the number
of planes, and I think returning the vector will result in more flexible
code for the caller.

> +
> +private:
> +	Status status_;
> +	unsigned int sequence_;
> +	uint64_t timestamp_;

Should this be a std::chrono type instead ?

> +	std::vector<PlaneInfo> planes_;
> +};
> +
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_BUFFER_H__ */

-- 
Regards,

Laurent Pinchart