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

2351 diff mbox series

For FrameBuffers the metadata information gathered when a buffer is
captured will not be stored directly in the buffer object but in its
own container. Add the BufferInfo class to hold this information.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/buffer.h | 30 +++++++++++++
 src/libcamera/buffer.cpp   | 92 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)

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@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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hello,

On Wed, Nov 27, 2019 at 11:12:36AM +0100, Jacopo Mondi wrote:
> 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.

Maybe

With the introduction of FrameBuffer objects, the metadata information
related to captured frames will not be stored directly in the frame
buffer object. Add a new BufferInfo class to hold this information.

as "buffer" is confusing in this context (does it refer to the new
FrameBuffer class or the existing Buffer class) ?

> > Signed-off-by: Niklas Söderlund <niklas.soderlund@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

Should this be named FrameInfo (or FrameMetadata), as it refers to the
frame captured in a buffer, not to the buffer itself ?

> > +{
> > +public:
> > +	enum Status {
> > +		BufferSuccess,
> > +		BufferError,
> > +		BufferCancelled,

s/Buffer/Frame/ here too ?

> > +	};
> 
> 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 ?

As Plane is qualified by BufferInfo I think this is fine.

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

I would prefer keeping that out of the BufferInfo class as V4L2
shouldn't be exposed in public headers. That being said, I agree with
the comment regarding the scalability issue with the update() method.
Maybe the solution would be to turn this class into a struct with public
members only ?

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

I think my only possible answer to this is Kamoulox, but that may be
difficult to explain for non-French speakers. The closest approximation
seems to be Mornington Crescent, which is not exactly the best match,
but leaves me in a similar state of bewilderment.

Anyway, if you agree with my above proposal to rename the class to
FrameInfo or FrameMetadata, this could become

 * \brief Metadata related to a captured frame
 *
 * The FrameInfo class stores all metadata related to a captured frame, as
 * stored in a FrameBuffer, such as capture status, timestamp or memory size.

> > + */
> > +
> > +/**
> > + * \enum BufferInfo::Status
> > + * Buffer completion status

Is this missing a \brief tag ?

> > + * \var BufferInfo::BufferSuccess
> > + * The buffer has completed with success and contains valid data. All its other

Maybe "The frame has been captured with success ..." ?

> > + * metadata (such as bytesused(), timestamp() or sequence() number) are valid.
> 
> I would drop "(such as...)"

"All other members of the FrameInfo class contain valid values."

These two modifications apply to the next two states below.

> > + * \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// ?

BufferInfo uses metadata in its \brief, BufferInfo::Plane should be
consistent, I think both should use metadata or information.

"Per-plane frame metadata" ?

I think a slightly longer description would also be useful after the
\brief. Something along the lines of

 * Frames are stored in memory in one or multiple planes. The FrameInfo::Plane
 * structure stores per-plane metadata for each plane.

> > + */
> > +
> > +/**
> > + * \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

It's not a method :-)

> > + */
> > +
> > +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;
> > +}

Given the implementation I really think the update() method makes little
sense, compared to either making the members public, or to creating a
constructor that takes all the above parameter and then using the
assignment operator. I would go for the former.

> > +
> > +/**
> > + * \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

When "the buffer was processed" or "when the frame was captured" ?

> > + *
> > + * The timestamp is expressed as a number of nanoseconds since the epoch.

That's actually not true, different kernel drivers use different clocks
:-S

We'll have to pay special attention to timestamps, they're quite a mess
today due to all the different implementations in the V4L2 API. For now
I think we could document this as

 * The timestamp is expressed as a number of nanoseconds relative to the system
 * clock since an unspecified time point.

and add a task to revisit this.

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

s/A array/An array/

> > + */
> > +
> 
> The patch direction is good, so fix whatever you consider approriate
> of the above comments and add
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> >  /**
> >   * \class Plane
> >   * \brief A memory region to store a single plane of a frame