[libcamera-devel] [PATCH 03/30] libcamera: buffer: Add BufferInfo container for buffer metadata information

[Jacopo Mondi]

Hi Niklas,

On Wed, Nov 27, 2019 at 12:35:53AM +0100, Niklas Söderlund wrote:
> For FrameBuffers the metadata information gathered when a buffer is
> captured will not be stored directly in the buffer object but in its

s/its/their if you pluralize FrameBuffers, which I won't...

> own container. Add the BufferInfo class to hold this information.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  include/libcamera/buffer.h | 30 +++++++++++++
>  src/libcamera/buffer.cpp   | 92 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 122 insertions(+)
>
> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> index 80602124f24be4a0..7302b9f32ce8c50d 100644
> --- a/include/libcamera/buffer.h
> +++ b/include/libcamera/buffer.h
> @@ -16,6 +16,36 @@ namespace libcamera {
>  class Request;
>  class Stream;
>
> +class BufferInfo
> +{
> +public:
> +	enum Status {
> +		BufferSuccess,
> +		BufferError,
> +		BufferCancelled,
> +	};

Is the BufferStatus part of the metadata or something that belongs to
the buffer itself ? Reading the documentation the "Success" or "Error"
states depend on the availability of valid image in the Buffer. I
think when I'll see it used probably this will be clarified.

> +
> +	struct Plane {
> +		unsigned int bytesused;
> +	};

Both the forthcoming FrameBuffer and BufferInfo have a "Plane" which
indeed represent two different things. What about PlaneInfo ?

> +
> +	BufferInfo();
> +
> +	void update(Status status, unsigned int sequence, uint64_t timestamp,
> +		    const std::vector<Plane> &planes);

Should we provide an utility method to update a BufferInfo with
information from a "struct v4l2_buffer" ? There won't be many users
apart from the V4L2VideoDevice class, and it's probably better to
centralize the procedure to extract information from a v4l2_buffer
there, but I fear BufferInfo::update() to grow its parameters list as
we increase the number of metadata it contains.

> +
> +	Status status() const { return status_; }
> +	unsigned int sequence() const { return sequence_; }
> +	uint64_t timestamp() const { return timestamp_; }
> +	const std::vector<Plane> &planes() const { return planes_; }
> +
> +private:
> +	Status status_;
> +	unsigned int sequence_;
> +	uint64_t timestamp_;
> +	std::vector<Plane> planes_;
> +};
> +
>  class Plane final
>  {
>  public:
> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> index 960eeb8f7d193ddd..4acff216cafba484 100644
> --- a/src/libcamera/buffer.cpp
> +++ b/src/libcamera/buffer.cpp
> @@ -23,6 +23,98 @@ namespace libcamera {
>
>  LOG_DEFINE_CATEGORY(Buffer)
>
> +/**
> + * \class BufferInfo
> + * \brief Dynamic buffer metadata
> + *
> + * The BufferInfo class references a buffers dynamic metadata related to the
> + * frame contained in the buffer.
> + */
> +
> +/**
> + * \enum BufferInfo::Status
> + * Buffer completion status
> + * \var BufferInfo::BufferSuccess
> + * The buffer has completed with success and contains valid data. All its other
> + * metadata (such as bytesused(), timestamp() or sequence() number) are valid.

I would drop "(such as...)"

> + * \var BufferInfo::BufferError
> + * The buffer has completed with an error and doesn't contain valid data. Its
> + * other metadata are valid.
> + * \var BufferInfo::BufferCancelled
> + * The buffer has been cancelled due to capture stop. Its other metadata are
> + * invalid and shall not be used.
> + */
> +
> +/**
> + * \struct BufferInfo::Plane
> + * \brief Plane specific buffer information

s/buffer// ?

> + */
> +
> +/**
> + * \var BufferInfo::Plane::bytesused
> + * \brief Retrieve the number of bytes occupied by the data in the plane
> + * \return Number of bytes occupied in the plane
> + */
> +
> +BufferInfo::BufferInfo()
> +	: status_(BufferError), sequence_(0), timestamp_(0)
> +{
> +}
> +
> +/**
> + * \brief Uptade the buffer information

s/Uptade/Update

> + * \param[in] status New buffer status
> + * \param[in] sequence New buffer sequence

sequence number

> + * \param[in] timestamp New buffer timestamp
> + * \param[in] planes New buffer plane meta data

s/New buffer/The buffer/ ?
> + *
> + * Update the stored buffer meta data information.
> + */
> +void BufferInfo::update(Status status, unsigned int sequence,
> +			uint64_t timestamp, const std::vector<Plane> &planes)
> +{
> +	status_ = status;
> +	sequence_ = sequence;
> +	timestamp_ = timestamp;
> +	planes_ = planes;
> +}
> +
> +/**
> + * \fn BufferInfo::status()
> + * \brief Retrieve the buffer status
> + *
> + * The buffer status reports whether the buffer has completed successfully
> + * (BufferSuccess) or if an error occurred (BufferError).

I think you either re-mention all states, or just point the
documentation to the BufferInfo::Status enumeration.

> + *
> + * \return The buffer status
> + */
> +
> +/**
> + * \fn BufferInfo::sequence()
> + * \brief Retrieve the buffer sequence number
> + *
> + * The sequence number is a monotonically increasing number assigned to the
> + * buffer processed by the stream. Gaps in the sequence numbers indicate
> + * dropped frames.
> + *
> + * \return Sequence number of the buffer
> + */
> +
> +/**
> + * \fn BufferInfo::timestamp()
> + * \brief Retrieve the time when the buffer was processed
> + *
> + * The timestamp is expressed as a number of nanoseconds since the epoch.
> + *
> + * \return Timestamp when the buffer was processed
> + */
> +
> +/**
> + * \fn BufferInfo::planes()
> + * \brief Retrieve the plane(s) information for the buffer
> + * \return A array holding all plane information for the buffer
> + */
> +

The patch direction is good, so fix whatever you consider approriate
of the above comments and add
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>  /**
>   * \class Plane
>   * \brief A memory region to store a single plane of a frame
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191127/225afb9a/attachment-0001.sig>