[{"id":27666,"web_url":"https://patchwork.libcamera.org/comment/27666/","msgid":"<169222278912.695434.15680632645164790080@ping.linuxembedded.co.uk>","date":"2023-08-16T21:53:09","subject":"Re: [libcamera-devel] [RFC PATCH 5/5] libcamera: pipeline:\n\tuvcvideo: Handle metadata stream","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Gabby George (2023-08-14 12:28:49)\n> Register the metadata stream's buffer ready callback and start\n> processing metadata buffers.  Use the timestamp from the metadata\n> buffer as the corresponding video buffer Requests' timestamp. Metadata\n> buffers are synchronized with frames coming into the video stream\n> using the sequence field of the buffers. They may come in either order\n> (video buffer first or metadata buffer first), so store relevant\n> information about the buffer required to set the metadata timestamp or\n> complete the buffer request as soon as possible.\n> \n> The timestamp will improved upon in the next patch. For now, use the\n\n'will be improved'\n\n> driver-provided metadata timestamp.\n> \n> Signed-off-by: Gabby George <gabbymg94@gmail.com>\n> ---\n>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 157 ++++++++++++++++++-\n>  1 file changed, 152 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 51f30187..5c7ae064 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -12,6 +12,8 @@\n>  #include <memory>\n>  #include <tuple>\n>  \n> +#include <linux/uvcvideo.h>\n> +\n>  #include <libcamera/base/log.h>\n>  #include <libcamera/base/utils.h>\n>  \n> @@ -34,6 +36,13 @@ namespace libcamera {\n>  \n>  LOG_DEFINE_CATEGORY(UVC)\n>  \n> +/* This is used to memcpy */\n\nWhy ? (The comment isn't helpful I'm afraid).\n\n> +struct UVCMetadataPacked {\n> +       __u32 pts;\n> +       __u32 scr;\n> +       __u16 sofDevice;\n> +} __attribute__((packed));\n> +\n>  class UVCCameraData : public Camera::Private\n>  {\n>  public:\n> @@ -46,6 +55,7 @@ public:\n>         void addControl(uint32_t cid, const ControlInfo &v4l2info,\n>                         ControlInfoMap::Map *ctrls);\n>         void bufferReady(FrameBuffer *buffer);\n> +       void bufferReadyMetadata(FrameBuffer *buffer);\n>  \n>         const std::string &id() const { return id_; }\n>  \n> @@ -57,10 +67,14 @@ public:\n>         bool useMetadataStream_;\n>  \n>         std::map<PixelFormat, std::vector<SizeRange>> formats_;\n> +       std::queue<std::pair<Request *, FrameBuffer *>> waitingForVideoBuffer_;\n> +       std::queue<std::pair<unsigned int, uint64_t>> waitingForMDBuffer_;\n\nI think I would have used waitingForMetaBuffer_ ... but this is probably\nfine.\n\n\n>  \n>  private:\n>         int initMetadata(MediaDevice *media);\n>  \n> +       const unsigned int frameStart_ = 1;\n> +       const unsigned int maxVidBuffersInQueue_ = 2;\n>         const unsigned int minLengthHeaderBuf_ = 10;\n>  \n>         bool generateId();\n> @@ -638,8 +652,16 @@ int UVCCameraData::init(MediaDevice *media)\n>         }\n>  \n>         controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> +       ret = initMetadata(media);\n> +\n> +       if (!ret) {\n> +               metadata_->bufferReady.connect(this, &UVCCameraData::bufferReadyMetadata);\n> +               useMetadataStream_ = true;\n> +       } else {\n> +               useMetadataStream_ = false;\n\nShould this release the metadata_ node? It won't be used now will it?\n(or can it on restarts?)\n\n\n> +       }\n>  \n> -       return initMetadata(media);\n> +       return 0;\n\nAha - this fixes an earlier comment at least. We might still be better\nfixing the earlier patch too though.\n\n>  }\n>  \n>  bool UVCCameraData::generateId()\n> @@ -824,16 +846,141 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n>         ctrls->emplace(id, info);\n>  }\n>  \n> +/*\n> + * If there is a metadata buffer that hasn't been matched with a\n> + * video buffer, check to see if it matches this video buffer.\n> + *\n> + * If there is a match, use the timestamp stored in the metadata queue\n> + * for this video buffer's request. Complete this video buffer\n> + * and its request.\n> + *\n> + * If there are no metadata buffers available to check for a match,\n> + * push this video buffer's request object to the queue. It may\n> + * be that the metadata buffer has not yet arrived.\n> + * When the matching metadata buffer does come in, it will handle\n> + * completion of the buffer and request.\n> + *\n> + * If more than maxVidBuffersInQueue_ video buffers have been added\n> + * to the queue, something is wrong with the metadata stream and\n> + * we can no longer use UVC metadata packets for timestamps.\n> + * Complete all of the outstanding requests and turn off metadata\n> + * stream use.\n> + */\n>  void UVCCameraData::bufferReady(FrameBuffer *buffer)\n>  {\n>         Request *request = buffer->request();\n> -\n> -       /* \\todo Use the UVC metadata to calculate a more precise timestamp */\n\nIt looks like we shouldn't actually remove this comment yet in this\npatch ...\n\n>         request->metadata().set(controls::SensorTimestamp,\n>                                 buffer->metadata().timestamp);\n>  \n> -       pipe()->completeBuffer(request, buffer);\n> -       pipe()->completeRequest(request);\n> +       if (useMetadataStream_) {\n> +               if (buffer->metadata().sequence == 0) {\n> +                       /* \\todo: we do not expect the first frame to have a\n> +                       * metadata buffer associated with it.  Why?\n> +                       */\n\n/* comment styles again.\n* like this.\n*/\n\n/*\n * Should be formatted like\n * this when on multi-lines\n */\n\n> +                       pipe()->completeBuffer(request, buffer);\n> +                       pipe()->completeRequest(request);\n> +                       return;\n> +               }\n> +\n> +               if (!waitingForMDBuffer_.empty()) {\n> +                       unsigned int mdSequence =\n> +                               std::get<0>(waitingForMDBuffer_.front()) + frameStart_;\n> +                       if (mdSequence == buffer->metadata().sequence) {\n> +                               request->metadata().set(controls::SensorTimestamp,\n> +                                                       std::get<1>(waitingForMDBuffer_.front()));\n> +                               pipe()->completeBuffer(request, buffer);\n> +                               pipe()->completeRequest(request);\n> +                               waitingForMDBuffer_.pop();\n> +                               return;\n> +                       }\n> +               } else {\n> +                       waitingForVideoBuffer_.push(std::make_pair(request, buffer));\n> +               }\n> +\n> +               if (waitingForVideoBuffer_.size() > maxVidBuffersInQueue_) {\n> +                       while (!waitingForVideoBuffer_.empty()) {\n> +                               Request *oldRequest = std::get<0>(waitingForVideoBuffer_.front());\n> +                               FrameBuffer *oldBuffer = std::get<1>(waitingForVideoBuffer_.front());\n> +                               oldRequest->metadata().set(controls::SensorTimestamp,\n> +                                                          oldBuffer->metadata().timestamp);\n> +                               pipe()->completeBuffer(oldRequest, oldBuffer);\n> +                               pipe()->completeRequest(oldRequest);\n> +                               waitingForVideoBuffer_.pop();\n> +                       }\n> +               }\n> +       } else {\n> +               pipe()->completeBuffer(request, buffer);\n> +               pipe()->completeRequest(request);\n> +       }\n> +}\n> +\n> +void UVCCameraData::bufferReadyMetadata(FrameBuffer *buffer)\n> +{\n> +       if (!useMetadataStream_ || buffer->metadata().status != FrameMetadata::Status::FrameSuccess) {\n> +               return;\n> +       }\n> +\n> +       /*\n> +        * The metadata stream always starts at seq 1 and libcamera sets the start sequence to 0,\n> +        * so it's necessary to add one to match this buffer with the correct\n> +        * video frame buffer.\n> +        *\n> +        * \\todo: Is there a better way to do this?  What is the root cause?\n> +        */\n> +       unsigned int mdSequence = buffer->metadata().sequence + frameStart_;\n> +       int pos = buffer->cookie();\n> +       /*\n> +        * A UVC Metadata Block length field contains size of\n> +        * the header buf, length field, and flags field.\n> +        */\n> +       uvc_meta_buf metadataBuf;\n> +       __u8 minLength = minLengthHeaderBuf_ +\n> +                        sizeof(metadataBuf.length) +\n> +                        sizeof(metadataBuf.flags);\n> +       size_t lenMDPacket = minLength + sizeof(metadataBuf.ns) + sizeof(metadataBuf.sof);\n\nThat all looks quite complicated to calculate the size of a structure.\n\nCan we do anything with sizeof(metadataBuf) instead?\nWhy is minLengthHeaderBuf_ separated out?\n\nDoes metadataBuf.length == sizeof(UVCMetadataPacked) ?\n\n> +       memcpy(&metadataBuf, mappedMetadataBuffers_.at(pos).planes()[0].data(), lenMDPacket);\n\nIs this memcpy a temporary thing ? Or do we always need to do it. We\nprobably need a block comment above the memcpy explaining /why/ we're\ncopying this instead of accessing it directly.\n\nIs this even safe? Is there any chance that this memcpy could overwrite\nthe space provided by metadataBuf ? (I suspect running valgrind might\ntrigger here).\n\n> +\n> +       if (metadataBuf.length < minLength) {\n> +               LOG(UVC, Error) << \"Received improper metadata packet.  Using default timestamps.\";\n> +               useMetadataStream_ = false;\n> +               return;\n> +       }\n> +\n> +       /*\n> +        * If there is a video buffer that hasn't been matched with a\n> +        * metadata buffer, check to see if it matches this metadata buffer.\n> +        *\n> +        * If there is a match, use the timestamp associated with this\n> +        * metadata buffer as the timestamp for the video buffer's request.\n> +        * Complete that video buffer and its request.\n> +        *\n> +        * If there are no video buffers, push this metadata buffer's\n> +        * sequence number and timestamp to a shared queue.  It may be that\n> +        * the metadata buffer came in before the video buffer.\n> +        * When the matching video buffer does come in, it will use this\n> +        * metadata buffer's timestamp.\n> +        */\n> +       __u64 timestamp = metadataBuf.ns;\n> +\n> +       if (!waitingForVideoBuffer_.empty()) {\n> +               Request *request = std::get<0>(waitingForVideoBuffer_.front());\n> +               FrameBuffer *vidBuffer = std::get<1>(waitingForVideoBuffer_.front());\n\nInteresting I haven't seen this syntax used. We normally use structured\nbindings for something like this, which I think would look like:\n\n\t\tauto [request, vidBuffer] = waitingForVideoBuffer_.front();\n\nBut I like that your version expresses the whole type for each variable.\n\n\n> +               unsigned int vidSequence = vidBuffer->metadata().sequence;\n> +\n> +               if (vidSequence == mdSequence) {\n> +                       request->metadata().set(controls::SensorTimestamp,\n> +                                               timestamp);\n> +\n> +                       pipe()->completeBuffer(request, vidBuffer);\n> +                       pipe()->completeRequest(request);\n> +                       waitingForVideoBuffer_.pop();\n> +               }\n\n\n\t\tif (vidSequence == mdSequence) {\n\t\t\t/* Your code above */\n\t\t} else {\n\t\t\tWhat happens here? Do we need to worry about anything in\n\t\t\there?\n\t\t}\n\n> +       } else {\n> +               waitingForMDBuffer_.push(\n> +                       std::make_pair(buffer->metadata().sequence,\n> +                                      timestamp));\n> +       }\n> +       metadata_->queueBuffer(buffer);\n>  }\n>  \n>  REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)\n> -- \n> 2.34.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CFEF9BF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 16 Aug 2023 21:53:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47254628D7;\n\tWed, 16 Aug 2023 23:53:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4A0A628D2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 16 Aug 2023 23:53:11 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 794FC2C6;\n\tWed, 16 Aug 2023 23:51:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692222793;\n\tbh=3kIVWov4QmJB7+h8l92OEdiQD6fKAaeLR9pgARB0Sio=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=X17Y9YXj1n/ek58Na/fRWTTc4HuPGSDhJ74SSNLPCmWg6soQcraQ+BD7yamo6kv1R\n\tMjuq+9sBUnsDAwat/57I/nIQ3wTL/4nrlYKFvIRIid853w3Du0AkceQMOIPj5XZ8ab\n\twfO2uwoy2vdaagcRH0LdQ/nFHtu03PAMIFYVRRuk6SGaWrpJ4Cnof4s86A1VQqGKoy\n\tk8/BgdOQRpv/yFoksHfK27YP3zSLbR7hugTZ45DcLxEWxmGIIiuv2xXwzwpQ9LrlRg\n\t8NAUTt+0ekWMAD4Ikld0j0bdQ1YGa0xIyyUjryZ3DaivWy2b5XJ5VoXG+4vgJDWi+j\n\tB9pink5byN5fQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1692222718;\n\tbh=3kIVWov4QmJB7+h8l92OEdiQD6fKAaeLR9pgARB0Sio=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=TGGJWF39yEpMxWTjxeK8UFJptH28R6inHNwNJmawzqy13SZm2vRH13MjBJ2MZNoem\n\tOWiBLkfxldBn+Ie0ymKDX9TN7b6yKqclY1K5YvEfJHSXESVx6EYuPdOiDD919a7mHG\n\tKcU3rPI9MMTdgW9GpHJxvy3t5MlgqMA1UkXCK3Ko="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TGGJWF39\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230814112849.176943-6-gabbymg94@gmail.com>","References":"<20230814112849.176943-1-gabbymg94@gmail.com>\n\t<20230814112849.176943-6-gabbymg94@gmail.com>","To":"gabbymg94@gmail.com, libcamera-devel@lists.libcamera.org,\n\tvedantparanjape160201@gmail.com","Date":"Wed, 16 Aug 2023 22:53:09 +0100","Message-ID":"<169222278912.695434.15680632645164790080@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC PATCH 5/5] libcamera: pipeline:\n\tuvcvideo: Handle metadata stream","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]