[{"id":3315,"web_url":"https://patchwork.libcamera.org/comment/3315/","msgid":"<20200103152249.bmwupm6xibsebhbr@uno.localdomain>","date":"2020-01-03T15:22:49","subject":"Re: [libcamera-devel] [PATCH v2 01/25] libcamera: buffer: Add\n\tFrameMetadata container for metadata information","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n some minor notes\n\nOn Mon, Dec 30, 2019 at 01:04:46PM +0100, Niklas Söderlund wrote:\n> With the introduction of FrameBuffer objects, the metadata information\n> related to captured frames will not be stored directly in the frame\n> buffer object. Add a new FrameMetadata class to hold this information.\n>\n> Signed-off-by: Niklas Söderlund \n> Reviewed-by: Jacopo Mondi \n> ---\n> * Changes since v1:\n> - Rename BufferInfo to FrameMetadata\n> - Rename prefix for BufferInfo::Status s/Buffer/Frame/\n> - Make FrameMetadata a struct with public data members instead of a\n> class with accessors methods.\n> - Dropped FrameMetadata::update()\n> - Documentation updates\n> ---\n> include/libcamera/buffer.h | 17 +++++++++++\n> src/libcamera/buffer.cpp | 62 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 79 insertions(+)\n>\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 80602124f24be4a0..0b95e41a2dd205b2 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -16,6 +16,23 @@ namespace libcamera {\n> class Request;\n> class Stream;\n>\n> +struct FrameMetadata {\n> +\tenum Status {\n> +\t\tFrameSuccess,\n> +\t\tFrameError,\n> +\t\tFrameCancelled,\n> +\t};\n> +\n> +\tstruct Plane {\n> +\t\tunsigned int bytesused;\n> +\t};\n> +\n> +\tStatus status;\n> +\tunsigned int sequence;\n> +\tuint64_t timestamp;\n> +\tstd::vector planes;\n> +};\n> +\n> class Plane final\n> {\n> public:\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 960eeb8f7d193ddd..7673b96f29728a24 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -23,6 +23,68 @@ namespace libcamera {\n>\n> LOG_DEFINE_CATEGORY(Buffer)\n>\n> +/**\n> + * \\struct FrameMetadata\n> + * \\brief Metadata related to a captured frame\n> + *\n> + * The FrameMetadata structure stores all metadata related to a captured frame,\n> + * as stored in a FrameBuffer, such as capture status, timestamp or memory size.\n> + */\n> +\n> +/**\n> + * \\enum FrameMetadata::Status\n> + * \\brief Define the frame completion status\n> + * \\var FrameMetadata::FrameSuccess\n> + * The frame has been captured with success and contains valid data and metadata\n> + * \\var FrameMetadata::FrameError\n> + * The frame has been captured with an error and doesn't contain valid metadata\n> + * \\var FrameMetadata::FrameCancelled\n> + * The frame has been cancelled and doesn't contain valid metadata\n> + */\n> +\n> +/**\n> + * \\struct FrameMetadata::Plane\n> + * \\brief Per-plane frame metadata\n> + *\n> + * Frames are stored in memory in one or multiple planes. The FrameInfo::Plane\n> + * structure stores per-plane metadata for each plane.\n\n\"per-plane metadata for each plane\" seems a bit a repetition maybe ?\n\n> + */\n> +\n> +/**\n> + * \\var FrameMetadata::Plane::bytesused\n> + * \\brief Number of bytes occupied by the data in the plane\n> + */\n> +\n> +/**\n> + * \\var FrameMetadata::status\n> + * \\brief Status of the frame\n> + *\n> + * Values in the container are only valid when the status is\n> + * FrameMetadata::FrameSuccess.\n\nIs \"container\" the FrameBuffer and \"Values\" the image data or is this\nthe metadata, or both ? I would specify it here\n\n> + */\n> +\n> +/**\n> + * \\var FrameMetadata::sequence\n> + * \\brief Frame sequence number\n> + *\n> + * The sequence number is a monotonically increasing number assigned to the\n> + * buffer processed by the stream. Gaps in the sequence numbers indicate\n\ns/processed/produced\n\n> + * dropped frames.\n> + */\n> +\n> +/**\n> + * \\var FrameMetadata::timestamp\n> + * \\brief Time when the frame was captured\n> + *\n> + * The timestamp is expressed as a number of nanoseconds relative to the system\n> + * clock since an unspecified time point.\n\nNot for this series, but I think we'll have to be more precise on what\ntimestamps refer to\n\n> + */\n> +\n> +/**\n> + * \\var FrameMetadata::planes\n> + * \\brief Array holding plane specific metadata\n\n\"Array of plane specific metadata\" ?\n\nPlease keep my tag.\n\nThanks\n j\n\n> + */\n> +\n> /**\n> * \\class Plane\n> * \\brief A memory region to store a single plane of a frame\n> --\n> 2.24.1\n>","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 35962605CD\n\tfor ;\n\tFri, 3 Jan 2020 16:20:32 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id B68861C0003;\n\tFri, 3 Jan 2020 15:20:31 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 3 Jan 2020 16:22:49 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200103152249.bmwupm6xibsebhbr@uno.localdomain>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"4t6yvgluhgasuejo\"","Content-Disposition":"inline","In-Reply-To":"<20191230120510.938333-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 01/25] libcamera: buffer: Add\n\tFrameMetadata container for metadata information","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 03 Jan 2020 15:20:32 -0000"}},{"id":3333,"web_url":"https://patchwork.libcamera.org/comment/3333/","msgid":"<20200106234240.GD22189@pendragon.ideasonboard.com>","date":"2020-01-06T23:42:40","subject":"Re: [libcamera-devel] [PATCH v2 01/25] libcamera: buffer: Add\n\tFrameMetadata container for metadata information","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Jan 03, 2020 at 04:22:49PM +0100, Jacopo Mondi wrote:\n> On Mon, Dec 30, 2019 at 01:04:46PM +0100, Niklas Söderlund wrote:\n> > With the introduction of FrameBuffer objects, the metadata information\n> > related to captured frames will not be stored directly in the frame\n> > buffer object. Add a new FrameMetadata class to hold this information.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > * Changes since v1:\n> > - Rename BufferInfo to FrameMetadata\n> > - Rename prefix for BufferInfo::Status s/Buffer/Frame/\n> > - Make FrameMetadata a struct with public data members instead of a\n> > class with accessors methods.\n> > - Dropped FrameMetadata::update()\n> > - Documentation updates\n> > ---\n> > include/libcamera/buffer.h | 17 +++++++++++\n> > src/libcamera/buffer.cpp | 62 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 79 insertions(+)\n> >\n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index 80602124f24be4a0..0b95e41a2dd205b2 100644\n> > --- a/include/libcamera/buffer.h\n> > +++ b/include/libcamera/buffer.h\n> > @@ -16,6 +16,23 @@ namespace libcamera {\n> > class Request;\n> > class Stream;\n> >\n> > +struct FrameMetadata {\n> > +\tenum Status {\n> > +\t\tFrameSuccess,\n> > +\t\tFrameError,\n> > +\t\tFrameCancelled,\n> > +\t};\n> > +\n> > +\tstruct Plane {\n> > +\t\tunsigned int bytesused;\n> > +\t};\n> > +\n> > +\tStatus status;\n> > +\tunsigned int sequence;\n> > +\tuint64_t timestamp;\n> > +\tstd::vector planes;\n\nI wonder if we shouldn't have a fixed-size array, that would be more\nefficient than a vector. Maybe an optimization target for later, but if\nit's easy (and if we agree on this change) we could already do so.\n\n> > +};\n> > +\n> > class Plane final\n> > {\n> > public:\n> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > index 960eeb8f7d193ddd..7673b96f29728a24 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -23,6 +23,68 @@ namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(Buffer)\n> >\n> > +/**\n> > + * \\struct FrameMetadata\n> > + * \\brief Metadata related to a captured frame\n> > + *\n> > + * The FrameMetadata structure stores all metadata related to a captured frame,\n> > + * as stored in a FrameBuffer, such as capture status, timestamp or memory size.\n\n\"memory size\" sounds like the size of the buffer. We may want to call\nthis differently. V4L2 uses length and bytesused, C++ std uses capacity\nand size, DRM doesn't use anything as it doesn't support variable-size\nformats.\n\nI'm not too fond of \"bytesused\" as it's hard to use in text. Going the\nC++ std way could generate confusion as we are used to \"size\" referring\nto the full buffer size. We could also use FrameBuffer::size for the\ncapacity and FrameMetadata::size for the used size, but that would\nprobably be even more confusing.\n\nThrowing in synonyms, there's \"stored\" (for \"used\") and \"content size\"\nthat come to my mind. Can anyone think of good options ?\n\n> > + */\n> > +\n> > +/**\n> > + * \\enum FrameMetadata::Status\n> > + * \\brief Define the frame completion status\n> > + * \\var FrameMetadata::FrameSuccess\n> > + * The frame has been captured with success and contains valid data and metadata\n\nWhat does it mean that a frame \"contains valid metadata\" ? Should this\nbe expressed as \"The frame has been captured with success and contains\nvalid data. All fields of the FrameMetadata structure are valid.\" ?\n\n> > + * \\var FrameMetadata::FrameError\n> > + * The frame has been captured with an error and doesn't contain valid metadata\n\n\"An error occurred during capture of the frame. The frame data may be\npartly or fully invalid. The sequence field of the FrameMetadata\nstructure is valid, the other fields may be invalid.\"\n\nAssuming we want to guarantee that sequence will always be valid. Can we\n? How about timestamp ? planes.bytesused is clearly invalid in this\ncase.\n\n> > + * \\var FrameMetadata::FrameCancelled\n> > + * The frame has been cancelled and doesn't contain valid metadata\n\n\"Capture stopped before the frame completed. The frame data is not\nvalid. .\"\n\nFor this case timestamp and planes.bytesused are invalid. How about\nsequence ?\n\n> > + */\n> > +\n> > +/**\n> > + * \\struct FrameMetadata::Plane\n> > + * \\brief Per-plane frame metadata\n> > + *\n> > + * Frames are stored in memory in one or multiple planes. The FrameInfo::Plane\n\ns/FrameInfo/FrameMetadata/\n\n> > + * structure stores per-plane metadata for each plane.\n> \n> \"per-plane metadata for each plane\" seems a bit a repetition maybe ?\n> \n> > + */\n> > +\n> > +/**\n> > + * \\var FrameMetadata::Plane::bytesused\n> > + * \\brief Number of bytes occupied by the data in the plane\n\nShould we add \", including line padding. This value may vary per frame\nfor compressed formats. For uncompressed formats it will be constant for\nall frames, but may be smaller than the FrameBuffer size.\" ?\n\n> > + */\n> > +\n> > +/**\n> > + * \\var FrameMetadata::status\n> > + * \\brief Status of the frame\n> > + *\n> > + * Values in the container are only valid when the status is\n> > + * FrameMetadata::FrameSuccess.\n> \n> Is \"container\" the FrameBuffer and \"Values\" the image data or is this\n> the metadata, or both ? I would specify it here\n\nAgreed. I think we can simply state that \"The validity of other fields\nof the FrameMetadata structure depends on the status value.\" if we\ndefine the status more precisely above.\n\n> > + */\n> > +\n> > +/**\n> > + * \\var FrameMetadata::sequence\n> > + * \\brief Frame sequence number\n> > + *\n> > + * The sequence number is a monotonically increasing number assigned to the\n> > + * buffer processed by the stream. Gaps in the sequence numbers indicate\n> \n> s/processed/produced\n> \n> > + * dropped frames.\n\ns/buffer processed/frames captured/ ? And adding between the two\nsentences \"The value is increased by one for each frame.\" ?\n\n> > + */\n> > +\n> > +/**\n> > + * \\var FrameMetadata::timestamp\n> > + * \\brief Time when the frame was captured\n> > + *\n> > + * The timestamp is expressed as a number of nanoseconds relative to the system\n> > + * clock since an unspecified time point.\n> \n> Not for this series, but I think we'll have to be more precise on what\n> timestamps refer to\n\nAbsolutely. Niklas, could you capture this in a \\todo ?\n\n> > + */\n> > +\n> > +/**\n> > + * \\var FrameMetadata::planes\n> > + * \\brief Array holding plane specific metadata\n> \n> \"Array of plane specific metadata\" ?\n\ns/plane specific/plane-specific/ or \"per-plane\".\n\nReviewed-by: Laurent Pinchart \n\n> Please keep my tag.\n> \n> > + */\n> > +\n> > /**\n> > * \\class Plane\n> > * \\brief A memory region to store a single plane of a frame","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B25F5605FB\n\tfor ;\n\tTue, 7 Jan 2020 00:42:51 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2C9E552F;\n\tTue, 7 Jan 2020 00:42:51 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1578354171;\n\tbh=SGaRQ4rBx3ZQbmR3OBMcKRu4HnQOtV6muB5iybgJOfY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r4w7n9xYnedEF6e+yTHQv0VWyE3Fp+vU3r3mZ0Vy4VJZ2BcRg5GfzvH6n6AALyid/\n\tlJXP/G/FfBS4oQwV0bmXvfyn2PTuyedXatWa+XOy37ypWhKt8h7f9DvrBY+H/6tUd1\n\twNZmk5EEV73Q7d4mFUzYCSOrEe6QVNPp8t/3/nDo=","Date":"Tue, 7 Jan 2020 01:42:40 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200106234240.GD22189@pendragon.ideasonboard.com>","References":"<20191230120510.938333-1-niklas.soderlund@ragnatech.se>\n\t<20191230120510.938333-2-niklas.soderlund@ragnatech.se>\n\t<20200103152249.bmwupm6xibsebhbr@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200103152249.bmwupm6xibsebhbr@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 01/25] libcamera: buffer: Add\n\tFrameMetadata container for metadata information","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 06 Jan 2020 23:42:51 -0000"}}]