[libcamera-devel] [PATCH v2 01/25] libcamera: buffer: Add FrameMetadata container for metadata information

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Jan 03, 2020 at 04:22:49PM +0100, Jacopo Mondi wrote:
> On Mon, Dec 30, 2019 at 01:04:46PM +0100, Niklas Söderlund wrote:
> > 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 FrameMetadata class to hold this information.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > * Changes since v1:
> > - Rename BufferInfo to FrameMetadata
> > - Rename prefix for BufferInfo::Status s/Buffer/Frame/
> > - Make FrameMetadata a struct with public data members instead of a
> >   class with accessors methods.
> > - Dropped FrameMetadata::update()
> > - Documentation updates
> > ---
> >  include/libcamera/buffer.h | 17 +++++++++++
> >  src/libcamera/buffer.cpp   | 62 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 79 insertions(+)
> >
> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > index 80602124f24be4a0..0b95e41a2dd205b2 100644
> > --- a/include/libcamera/buffer.h
> > +++ b/include/libcamera/buffer.h
> > @@ -16,6 +16,23 @@ namespace libcamera {
> >  class Request;
> >  class Stream;
> >
> > +struct FrameMetadata {
> > +	enum Status {
> > +		FrameSuccess,
> > +		FrameError,
> > +		FrameCancelled,
> > +	};
> > +
> > +	struct Plane {
> > +		unsigned int bytesused;
> > +	};
> > +
> > +	Status status;
> > +	unsigned int sequence;
> > +	uint64_t timestamp;
> > +	std::vector<Plane> planes;

I wonder if we shouldn't have a fixed-size array, that would be more
efficient than a vector. Maybe an optimization target for later, but if
it's easy (and if we agree on this change) we could already do so.

> > +};
> > +
> >  class Plane final
> >  {
> >  public:
> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
> > index 960eeb8f7d193ddd..7673b96f29728a24 100644
> > --- a/src/libcamera/buffer.cpp
> > +++ b/src/libcamera/buffer.cpp
> > @@ -23,6 +23,68 @@ namespace libcamera {
> >
> >  LOG_DEFINE_CATEGORY(Buffer)
> >
> > +/**
> > + * \struct FrameMetadata
> > + * \brief Metadata related to a captured frame
> > + *
> > + * The FrameMetadata structure stores all metadata related to a captured frame,
> > + * as stored in a FrameBuffer, such as capture status, timestamp or memory size.

"memory size" sounds like the size of the buffer. We may want to call
this differently. V4L2 uses length and bytesused, C++ std uses capacity
and size, DRM doesn't use anything as it doesn't support variable-size
formats.

I'm not too fond of "bytesused" as it's hard to use in text. Going the
C++ std way could generate confusion as we are used to "size" referring
to the full buffer size. We could also use FrameBuffer::size for the
capacity and FrameMetadata::size for the used size, but that would
probably be even more confusing.

Throwing in synonyms, there's "stored" (for "used") and "content size"
that come to my mind. Can anyone think of good options ?

> > + */
> > +
> > +/**
> > + * \enum FrameMetadata::Status
> > + * \brief Define the frame completion status
> > + * \var FrameMetadata::FrameSuccess
> > + * The frame has been captured with success and contains valid data and metadata

What does it mean that a frame "contains valid metadata" ? Should this
be expressed as "The frame has been captured with success and contains
valid data. All fields of the FrameMetadata structure are valid." ?

> > + * \var FrameMetadata::FrameError
> > + * The frame has been captured with an error and doesn't contain valid metadata

"An error occurred during capture of the frame. The frame data may be
partly or fully invalid. The sequence field of the FrameMetadata
structure is valid, the other fields may be invalid."

Assuming we want to guarantee that sequence will always be valid. Can we
? How about timestamp ? planes.bytesused is clearly invalid in this
case.

> > + * \var FrameMetadata::FrameCancelled
> > + * The frame has been cancelled and doesn't contain valid metadata

"Capture stopped before the frame completed. The frame data is not
valid. <insert here explanation about other fields of FrameMetadata>."

For this case timestamp and planes.bytesused are invalid. How about
sequence ?

> > + */
> > +
> > +/**
> > + * \struct FrameMetadata::Plane
> > + * \brief Per-plane frame metadata
> > + *
> > + * Frames are stored in memory in one or multiple planes. The FrameInfo::Plane

s/FrameInfo/FrameMetadata/

> > + * structure stores per-plane metadata for each plane.
> 
> "per-plane metadata for each plane" seems a bit a repetition maybe ?
> 
> > + */
> > +
> > +/**
> > + * \var FrameMetadata::Plane::bytesused
> > + * \brief Number of bytes occupied by the data in the plane

Should we add ", including line padding. This value may vary per frame
for compressed formats. For uncompressed formats it will be constant for
all frames, but may be smaller than the FrameBuffer size." ?

> > + */
> > +
> > +/**
> > + * \var FrameMetadata::status
> > + * \brief Status of the frame
> > + *
> > + * Values in the container are only valid when the status is
> > + * FrameMetadata::FrameSuccess.
> 
> Is "container" the FrameBuffer and "Values" the image data or is this
> the metadata, or both ? I would specify it here

Agreed. I think we can simply state that "The validity of other fields
of the FrameMetadata structure depends on the status value." if we
define the status more precisely above.

> > + */
> > +
> > +/**
> > + * \var FrameMetadata::sequence
> > + * \brief Frame sequence number
> > + *
> > + * The sequence number is a monotonically increasing number assigned to the
> > + * buffer processed by the stream. Gaps in the sequence numbers indicate
> 
> s/processed/produced
> 
> > + * dropped frames.

s/buffer processed/frames captured/ ? And adding between the two
sentences "The value is increased by one for each frame." ?

> > + */
> > +
> > +/**
> > + * \var FrameMetadata::timestamp
> > + * \brief Time when the frame was captured
> > + *
> > + * The timestamp is expressed as a number of nanoseconds relative to the system
> > + * clock since an unspecified time point.
> 
> Not for this series, but I think we'll have to be more precise on what
> timestamps refer to

Absolutely. Niklas, could you capture this in a \todo ?

> > + */
> > +
> > +/**
> > + * \var FrameMetadata::planes
> > + * \brief Array holding plane specific metadata
> 
> "Array of plane specific metadata" ?

s/plane specific/plane-specific/ or "per-plane".

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> Please keep my tag.
> 
> > + */
> > +
> >  /**
> >   * \class Plane
> >   * \brief A memory region to store a single plane of a frame

-- 
Regards,

Laurent Pinchart