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

[Jacopo Mondi]

Hi Niklas,
  some minor notes

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;
> +};
> +
>  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.
> + */
> +
> +/**
> + * \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
> + * \var FrameMetadata::FrameError
> + * The frame has been captured with an error and doesn't contain valid metadata
> + * \var FrameMetadata::FrameCancelled
> + * The frame has been cancelled and doesn't contain valid metadata
> + */
> +
> +/**
> + * \struct FrameMetadata::Plane
> + * \brief Per-plane frame metadata
> + *
> + * Frames are stored in memory in one or multiple planes. The FrameInfo::Plane
> + * 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
> + */
> +
> +/**
> + * \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

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

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

"Array of plane specific metadata" ?

Please keep my tag.

Thanks
  j

> + */
> +
>  /**
>   * \class Plane
>   * \brief A memory region to store a single plane of a frame
> --
> 2.24.1
>
-------------- 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/20200103/f414314e/attachment.sig>