[{"id":3143,"web_url":"https://patchwork.libcamera.org/comment/3143/","msgid":"<20191127101236.ockej5alg43zcdbg@uno.localdomain>","date":"2019-11-27T10:12:36","subject":"Re: [libcamera-devel] [PATCH 03/30] libcamera: buffer: Add\n\tBufferInfo container for buffer metadata information","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Nov 27, 2019 at 12:35:53AM +0100, Niklas Söderlund wrote:\n> For FrameBuffers the metadata information gathered when a buffer is\n> captured will not be stored directly in the buffer object but in its\n\ns/its/their if you pluralize FrameBuffers, which I won't...\n\n> own container. Add the BufferInfo class to hold this information.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/buffer.h | 30 +++++++++++++\n> src/libcamera/buffer.cpp | 92 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 122 insertions(+)\n>\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index 80602124f24be4a0..7302b9f32ce8c50d 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -16,6 +16,36 @@ namespace libcamera {\n> class Request;\n> class Stream;\n>\n> +class BufferInfo\n> +{\n> +public:\n> +\tenum Status {\n> +\t\tBufferSuccess,\n> +\t\tBufferError,\n> +\t\tBufferCancelled,\n> +\t};\n\nIs the BufferStatus part of the metadata or something that belongs to\nthe buffer itself ? Reading the documentation the \"Success\" or \"Error\"\nstates depend on the availability of valid image in the Buffer. I\nthink when I'll see it used probably this will be clarified.\n\n> +\n> +\tstruct Plane {\n> +\t\tunsigned int bytesused;\n> +\t};\n\nBoth the forthcoming FrameBuffer and BufferInfo have a \"Plane\" which\nindeed represent two different things. What about PlaneInfo ?\n\n> +\n> +\tBufferInfo();\n> +\n> +\tvoid update(Status status, unsigned int sequence, uint64_t timestamp,\n> +\t\t const std::vector &planes);\n\nShould we provide an utility method to update a BufferInfo with\ninformation from a \"struct v4l2_buffer\" ? There won't be many users\napart from the V4L2VideoDevice class, and it's probably better to\ncentralize the procedure to extract information from a v4l2_buffer\nthere, but I fear BufferInfo::update() to grow its parameters list as\nwe increase the number of metadata it contains.\n\n> +\n> +\tStatus status() const { return status_; }\n> +\tunsigned int sequence() const { return sequence_; }\n> +\tuint64_t timestamp() const { return timestamp_; }\n> +\tconst std::vector &planes() const { return planes_; }\n> +\n> +private:\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..4acff216cafba484 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -23,6 +23,98 @@ namespace libcamera {\n>\n> LOG_DEFINE_CATEGORY(Buffer)\n>\n> +/**\n> + * \\class BufferInfo\n> + * \\brief Dynamic buffer metadata\n> + *\n> + * The BufferInfo class references a buffers dynamic metadata related to the\n> + * frame contained in the buffer.\n> + */\n> +\n> +/**\n> + * \\enum BufferInfo::Status\n> + * Buffer completion status\n> + * \\var BufferInfo::BufferSuccess\n> + * The buffer has completed with success and contains valid data. All its other\n> + * metadata (such as bytesused(), timestamp() or sequence() number) are valid.\n\nI would drop \"(such as...)\"\n\n> + * \\var BufferInfo::BufferError\n> + * The buffer has completed with an error and doesn't contain valid data. Its\n> + * other metadata are valid.\n> + * \\var BufferInfo::BufferCancelled\n> + * The buffer has been cancelled due to capture stop. Its other metadata are\n> + * invalid and shall not be used.\n> + */\n> +\n> +/**\n> + * \\struct BufferInfo::Plane\n> + * \\brief Plane specific buffer information\n\ns/buffer// ?\n\n> + */\n> +\n> +/**\n> + * \\var BufferInfo::Plane::bytesused\n> + * \\brief Retrieve the number of bytes occupied by the data in the plane\n> + * \\return Number of bytes occupied in the plane\n> + */\n> +\n> +BufferInfo::BufferInfo()\n> +\t: status_(BufferError), sequence_(0), timestamp_(0)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Uptade the buffer information\n\ns/Uptade/Update\n\n> + * \\param[in] status New buffer status\n> + * \\param[in] sequence New buffer sequence\n\nsequence number\n\n> + * \\param[in] timestamp New buffer timestamp\n> + * \\param[in] planes New buffer plane meta data\n\ns/New buffer/The buffer/ ?\n> + *\n> + * Update the stored buffer meta data information.\n> + */\n> +void BufferInfo::update(Status status, unsigned int sequence,\n> +\t\t\tuint64_t timestamp, const std::vector &planes)\n> +{\n> +\tstatus_ = status;\n> +\tsequence_ = sequence;\n> +\ttimestamp_ = timestamp;\n> +\tplanes_ = planes;\n> +}\n> +\n> +/**\n> + * \\fn BufferInfo::status()\n> + * \\brief Retrieve the buffer status\n> + *\n> + * The buffer status reports whether the buffer has completed successfully\n> + * (BufferSuccess) or if an error occurred (BufferError).\n\nI think you either re-mention all states, or just point the\ndocumentation to the BufferInfo::Status enumeration.\n\n> + *\n> + * \\return The buffer status\n> + */\n> +\n> +/**\n> + * \\fn BufferInfo::sequence()\n> + * \\brief Retrieve the buffer 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> + * dropped frames.\n> + *\n> + * \\return Sequence number of the buffer\n> + */\n> +\n> +/**\n> + * \\fn BufferInfo::timestamp()\n> + * \\brief Retrieve the time when the buffer was processed\n> + *\n> + * The timestamp is expressed as a number of nanoseconds since the epoch.\n> + *\n> + * \\return Timestamp when the buffer was processed\n> + */\n> +\n> +/**\n> + * \\fn BufferInfo::planes()\n> + * \\brief Retrieve the plane(s) information for the buffer\n> + * \\return A array holding all plane information for the buffer\n> + */\n> +\n\nThe patch direction is good, so fix whatever you consider approriate\nof the above comments and add\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> /**\n> * \\class Plane\n> * \\brief A memory region to store a single plane of a frame\n> --\n> 2.24.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C651760BB8\n\tfor ;\n\tWed, 27 Nov 2019 11:10:29 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 3120C240002;\n\tWed, 27 Nov 2019 10:10:28 +0000 (UTC)"],"Date":"Wed, 27 Nov 2019 11:12:36 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191127101236.ockej5alg43zcdbg@uno.localdomain>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"64pn5fknhmqvztqr\"","Content-Disposition":"inline","In-Reply-To":"<20191126233620.1695316-4-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 03/30] libcamera: buffer: Add\n\tBufferInfo container for buffer 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":"Wed, 27 Nov 2019 10:10:29 -0000"}},{"id":3226,"web_url":"https://patchwork.libcamera.org/comment/3226/","msgid":"<20191209174252.GF4853@pendragon.ideasonboard.com>","date":"2019-12-09T17:42:52","subject":"Re: [libcamera-devel] [PATCH 03/30] libcamera: buffer: Add\n\tBufferInfo container for buffer metadata information","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Nov 27, 2019 at 11:12:36AM +0100, Jacopo Mondi wrote:\n> On Wed, Nov 27, 2019 at 12:35:53AM +0100, Niklas Söderlund wrote:\n> > For FrameBuffers the metadata information gathered when a buffer is\n> > captured will not be stored directly in the buffer object but in its\n> \n> s/its/their if you pluralize FrameBuffers, which I won't...\n> \n> > own container. Add the BufferInfo class to hold this information.\n\nMaybe\n\nWith the introduction of FrameBuffer objects, the metadata information\nrelated to captured frames will not be stored directly in the frame\nbuffer object. Add a new BufferInfo class to hold this information.\n\nas \"buffer\" is confusing in this context (does it refer to the new\nFrameBuffer class or the existing Buffer class) ?\n\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/buffer.h | 30 +++++++++++++\n> > src/libcamera/buffer.cpp | 92 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 122 insertions(+)\n> >\n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index 80602124f24be4a0..7302b9f32ce8c50d 100644\n> > --- a/include/libcamera/buffer.h\n> > +++ b/include/libcamera/buffer.h\n> > @@ -16,6 +16,36 @@ namespace libcamera {\n> > class Request;\n> > class Stream;\n> >\n> > +class BufferInfo\n\nShould this be named FrameInfo (or FrameMetadata), as it refers to the\nframe captured in a buffer, not to the buffer itself ?\n\n> > +{\n> > +public:\n> > +\tenum Status {\n> > +\t\tBufferSuccess,\n> > +\t\tBufferError,\n> > +\t\tBufferCancelled,\n\ns/Buffer/Frame/ here too ?\n\n> > +\t};\n> \n> Is the BufferStatus part of the metadata or something that belongs to\n> the buffer itself ? Reading the documentation the \"Success\" or \"Error\"\n> states depend on the availability of valid image in the Buffer. I\n> think when I'll see it used probably this will be clarified.\n> \n> > +\n> > +\tstruct Plane {\n> > +\t\tunsigned int bytesused;\n> > +\t};\n> \n> Both the forthcoming FrameBuffer and BufferInfo have a \"Plane\" which\n> indeed represent two different things. What about PlaneInfo ?\n\nAs Plane is qualified by BufferInfo I think this is fine.\n\n> > +\n> > +\tBufferInfo();\n> > +\n> > +\tvoid update(Status status, unsigned int sequence, uint64_t timestamp,\n> > +\t\t const std::vector &planes);\n> \n> Should we provide an utility method to update a BufferInfo with\n> information from a \"struct v4l2_buffer\" ? There won't be many users\n> apart from the V4L2VideoDevice class, and it's probably better to\n> centralize the procedure to extract information from a v4l2_buffer\n> there, but I fear BufferInfo::update() to grow its parameters list as\n> we increase the number of metadata it contains.\n\nI would prefer keeping that out of the BufferInfo class as V4L2\nshouldn't be exposed in public headers. That being said, I agree with\nthe comment regarding the scalability issue with the update() method.\nMaybe the solution would be to turn this class into a struct with public\nmembers only ?\n\n> > +\n> > +\tStatus status() const { return status_; }\n> > +\tunsigned int sequence() const { return sequence_; }\n> > +\tuint64_t timestamp() const { return timestamp_; }\n> > +\tconst std::vector &planes() const { return planes_; }\n> > +\n> > +private:\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..4acff216cafba484 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -23,6 +23,98 @@ namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(Buffer)\n> >\n> > +/**\n> > + * \\class BufferInfo\n> > + * \\brief Dynamic buffer metadata\n> > + *\n> > + * The BufferInfo class references a buffers dynamic metadata related to the\n> > + * frame contained in the buffer.\n\nI think my only possible answer to this is Kamoulox, but that may be\ndifficult to explain for non-French speakers. The closest approximation\nseems to be Mornington Crescent, which is not exactly the best match,\nbut leaves me in a similar state of bewilderment.\n\nAnyway, if you agree with my above proposal to rename the class to\nFrameInfo or FrameMetadata, this could become\n\n * \\brief Metadata related to a captured frame\n *\n * The FrameInfo class stores all metadata related to a captured frame, as\n * stored in a FrameBuffer, such as capture status, timestamp or memory size.\n\n> > + */\n> > +\n> > +/**\n> > + * \\enum BufferInfo::Status\n> > + * Buffer completion status\n\nIs this missing a \\brief tag ?\n\n> > + * \\var BufferInfo::BufferSuccess\n> > + * The buffer has completed with success and contains valid data. All its other\n\nMaybe \"The frame has been captured with success ...\" ?\n\n> > + * metadata (such as bytesused(), timestamp() or sequence() number) are valid.\n> \n> I would drop \"(such as...)\"\n\n\"All other members of the FrameInfo class contain valid values.\"\n\nThese two modifications apply to the next two states below.\n\n> > + * \\var BufferInfo::BufferError\n> > + * The buffer has completed with an error and doesn't contain valid data. Its\n> > + * other metadata are valid.\n> > + * \\var BufferInfo::BufferCancelled\n> > + * The buffer has been cancelled due to capture stop. Its other metadata are\n> > + * invalid and shall not be used.\n> > + */\n> > +\n> > +/**\n> > + * \\struct BufferInfo::Plane\n> > + * \\brief Plane specific buffer information\n> \n> s/buffer// ?\n\nBufferInfo uses metadata in its \\brief, BufferInfo::Plane should be\nconsistent, I think both should use metadata or information.\n\n\"Per-plane frame metadata\" ?\n\nI think a slightly longer description would also be useful after the\n\\brief. Something along the lines of\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> > + */\n> > +\n> > +/**\n> > + * \\var BufferInfo::Plane::bytesused\n> > + * \\brief Retrieve the number of bytes occupied by the data in the plane\n> > + * \\return Number of bytes occupied in the plane\n\nIt's not a method :-)\n\n> > + */\n> > +\n> > +BufferInfo::BufferInfo()\n> > +\t: status_(BufferError), sequence_(0), timestamp_(0)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Uptade the buffer information\n> \n> s/Uptade/Update\n> \n> > + * \\param[in] status New buffer status\n> > + * \\param[in] sequence New buffer sequence\n> \n> sequence number\n> \n> > + * \\param[in] timestamp New buffer timestamp\n> > + * \\param[in] planes New buffer plane meta data\n> \n> s/New buffer/The buffer/ ?\n> > + *\n> > + * Update the stored buffer meta data information.\n> > + */\n> > +void BufferInfo::update(Status status, unsigned int sequence,\n> > +\t\t\tuint64_t timestamp, const std::vector &planes)\n> > +{\n> > +\tstatus_ = status;\n> > +\tsequence_ = sequence;\n> > +\ttimestamp_ = timestamp;\n> > +\tplanes_ = planes;\n> > +}\n\nGiven the implementation I really think the update() method makes little\nsense, compared to either making the members public, or to creating a\nconstructor that takes all the above parameter and then using the\nassignment operator. I would go for the former.\n\n> > +\n> > +/**\n> > + * \\fn BufferInfo::status()\n> > + * \\brief Retrieve the buffer status\n> > + *\n> > + * The buffer status reports whether the buffer has completed successfully\n> > + * (BufferSuccess) or if an error occurred (BufferError).\n> \n> I think you either re-mention all states, or just point the\n> documentation to the BufferInfo::Status enumeration.\n> \n> > + *\n> > + * \\return The buffer status\n> > + */\n> > +\n> > +/**\n> > + * \\fn BufferInfo::sequence()\n> > + * \\brief Retrieve the buffer 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> > + * dropped frames.\n> > + *\n> > + * \\return Sequence number of the buffer\n> > + */\n> > +\n> > +/**\n> > + * \\fn BufferInfo::timestamp()\n> > + * \\brief Retrieve the time when the buffer was processed\n\nWhen \"the buffer was processed\" or \"when the frame was captured\" ?\n\n> > + *\n> > + * The timestamp is expressed as a number of nanoseconds since the epoch.\n\nThat's actually not true, different kernel drivers use different clocks\n:-S\n\nWe'll have to pay special attention to timestamps, they're quite a mess\ntoday due to all the different implementations in the V4L2 API. For now\nI think we could document this as\n\n * The timestamp is expressed as a number of nanoseconds relative to the system\n * clock since an unspecified time point.\n\nand add a task to revisit this.\n\n> > + *\n> > + * \\return Timestamp when the buffer was processed\n> > + */\n> > +\n> > +/**\n> > + * \\fn BufferInfo::planes()\n> > + * \\brief Retrieve the plane(s) information for the buffer\n> > + * \\return A array holding all plane information for the buffer\n\ns/A array/An array/\n\n> > + */\n> > +\n> \n> The patch direction is good, so fix whatever you consider approriate\n> of the above comments and add\n> Reviewed-by: Jacopo Mondi \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 0924160BDB\n\tfor ;\n\tMon, 9 Dec 2019 18:43:00 +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 6902211B7;\n\tMon, 9 Dec 2019 18:42:59 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575913379;\n\tbh=FJVYr1fq8ZprmZI8rME6flA5rBTTDm9P8MoI8lyhvaQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ajns+8UEj6BZh1JW1tFweoagXcTfgb4xyb/A7Ul/+a8j75zT0J6rHfRY8+0WeVaxt\n\tdw4ZCkEQmSBBSjRyRzHFddrbBf+M5OlYePD8KeZ4QdzxIc0ddzkv86/KO6pg0/b0mq\n\tHZpjOa+YT4pEleZt55wRGrWwqATczdR9DGMqQJMc=","Date":"Mon, 9 Dec 2019 19:42:52 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20191209174252.GF4853@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-4-niklas.soderlund@ragnatech.se>\n\t<20191127101236.ockej5alg43zcdbg@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191127101236.ockej5alg43zcdbg@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 03/30] libcamera: buffer: Add\n\tBufferInfo container for buffer 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, 09 Dec 2019 17:43:00 -0000"}}]