[{"id":27681,"web_url":"https://patchwork.libcamera.org/comment/27681/","msgid":"<169265498017.435850.9404456795184764029@ping.linuxembedded.co.uk>","date":"2023-08-21T21:56:20","subject":"Re: [libcamera-devel] [RFC PATCH v2 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-21 14:10:39)\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 be improved upon in the next patch. For now, use\n> the driver-provided metadata timestamp.\n> \n> Signed-off-by: Gabby George \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 168 ++++++++++++++++++-\n> 1 file changed, 162 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index c8d6633f..215435ec 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -12,6 +12,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> \n> @@ -34,6 +36,17 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(UVC)\n> \n> +/*\n> + * The UVCH buffer contains an unsigned char array\n> + * encoding UVC timing data that needs to be recast\n> + * into usable data.\n> + */\n> +struct UVCTimingBuf {\n> + __u32 pts;\n> + __u32 stc;\n> + __u16 sofDevice;\n> +} __attribute__((packed));\n> +\n> class UVCCameraData : public Camera::Private\n> {\n> public:\n> @@ -46,6 +59,8 @@ 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> + void handleUnfinishedRequests();\n> \n> const std::string &id() const { return id_; }\n> \n> @@ -56,9 +71,16 @@ public:\n> std::map mappedMetadataBuffers_;\n> \n> std::map> formats_;\n> + std::queue pendingVideoBuffers_;\n> + std::queue> pendingMetadata_;\n> \n> private:\n> int initMetadata(MediaDevice *media);\n> + void completeRequest(FrameBuffer *buffer, uint64_t timestamp);\n> + void endCorruptedStream();\n> +\n> + const unsigned int frameStart_ = 1;\n> + const unsigned int maxVidBuffersInQueue_ = 1;\n> \n> bool generateId();\n> \n> @@ -242,7 +264,7 @@ int PipelineHandlerUVC::cleanupMetadataBuffers(Camera *camera)\n> UVCCameraData *data = cameraData(camera);\n> \n> if (data->metadata_)\n> - data->metadata_->releaseBuffers();\n> + ret = data->metadata_->releaseBuffers();\n\nI don't think this change is specific to this patch, so it shouldn't be\nhere, but I don't think it would squash to a previous patch anyway as I\nalready mentioned the cleanup functions may as well be void cleanup();\n\n> data->metadataBuffers_.clear();\n> data->mappedMetadataBuffers_.clear();\n> data->metadata_ = nullptr;\n> @@ -253,7 +275,9 @@ int PipelineHandlerUVC::cleanupMetadataBuffers(Camera *camera)\n> int PipelineHandlerUVC::cleanup(Camera *camera)\n> {\n> UVCCameraData *data = cameraData(camera);\n> +\n> Y cleanupMetadataBuffers(camera);\n> +\n> data->video_->releaseBuffers();\n> return 0;\n> }\n> @@ -354,6 +378,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n> if (data->metadata_)\n> data->metadata_->streamOff();\n> \n> + data->handleUnfinishedRequests();\n> +\n> cleanup(camera);\n> }\n> \n> @@ -646,8 +672,11 @@ int UVCCameraData::init(MediaDevice *media)\n> if (ret) {\n> metadata_ = nullptr;\n> LOG(UVC, Error) << \"Could not find a metadata video device.\";\n> + return 0;\n> }\n> \n> + metadata_->bufferReady.connect(this, &UVCCameraData::bufferReadyMetadata);\n> +\n> return 0;\n> }\n> \n> @@ -833,18 +862,145 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> ctrls->emplace(id, info);\n> }\n> \n> -void UVCCameraData::bufferReady(FrameBuffer *buffer)\n> +void UVCCameraData::completeRequest(FrameBuffer *buffer, uint64_t timestamp)\n> {\n> Request *request = buffer->request();\n> -\n> - /* \\todo Use the UVC metadata to calculate a more precise timestamp */\n> - request->metadata().set(controls::SensorTimestamp,\n> - buffer->metadata().timestamp);\n> + request->metadata().set(controls::SensorTimestamp, timestamp);\n> \n> pipe()->completeBuffer(request, buffer);\n> pipe()->completeRequest(request);\n> }\n> \n> +void UVCCameraData::handleUnfinishedRequests()\n> +{\n> + while (!pendingVideoBuffers_.empty()) {\n> + FrameBuffer *oldBuffer = pendingVideoBuffers_.front();\n> + Request *oldRequest = oldBuffer->request();\n> +\n> + oldRequest->metadata().set(controls::SensorTimestamp,\n> + oldBuffer->metadata().timestamp);\n> +\n> + pipe()->completeBuffer(oldRequest, oldBuffer);\n> + pipe()->completeRequest(oldRequest);\n\nShould this just call\n\tcompleteRequest(oldBuffer, oldBuffer->metadata().timestamp); ?\n\n> + pendingVideoBuffers_.pop();\n> + }\n> +}\n> +\n> +void UVCCameraData::endCorruptedStream()\n> +{\n> + handleUnfinishedRequests();\n> + /* Close the metadata node so we don't get inaccurate timestamps*/\n> + metadata_ = nullptr;\n> + LOG(UVC, Error)\n> + << \"UVC metadata stream corrupted. Reverting to driver timestamps.\";\n\nAha - so actually this doesn't stop the whole camera streaming - it just\nstops the metadata node?\n\nBefore closing the metadata_ should it release buffers and clean up\nthose ?\n\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> + /* \\todo Use the UVC metadata to calculate a more precise timestamp */\n> + if (!metadata_ || buffer->metadata().sequence < frameStart_) {\n> + completeRequest(buffer, buffer->metadata().timestamp);\n> + return;\n> + }\n> +\n> + if (!pendingMetadata_.empty()) {\n> + /* A metadata buffer was ready first. */\n> + unsigned int mdSequence = std::get<0>(pendingMetadata_.front()) + frameStart_;\n> + if (mdSequence == buffer->metadata().sequence) {\n> + completeRequest(buffer, std::get<1>(pendingMetadata_.front()));\n> + pendingMetadata_.pop();\n> + return;\n> + } else {\n> + /* \\todo: Is there a reason metadata buffers can arrive out of order? */\n> + endCorruptedStream();\n> + return;\n> + }\n> + }\n> +\n> + pendingVideoBuffers_.push(buffer);\n> + /*\n> + * Deal with video buffers that haven't been completed, and with\n> + * buffers whose metadata information arrived out of order.\n> + */\n> + if (pendingVideoBuffers_.size() > maxVidBuffersInQueue_) {\n> + endCorruptedStream();\n\nI'm worried about all cases this patch now introduces that seem to now\nmean we'll abort a running camera stream. I wonder if that's just part\nof the complexity introduced by having metadata delivered in multiple\nvideo nodes though.\n\n> + }\n> +}\n> +\n> +void UVCCameraData::bufferReadyMetadata(FrameBuffer *buffer)\n> +{\n> + if (!metadata_ ||\n> + buffer->metadata().status != FrameMetadata::Status::FrameSuccess) {\n> + return;\n> + }\n> +\n> + /*\n> + * The metadata stream seems to start at seq 1 and libcamera\n> + * sets the start sequence to 0, so it's necessary to add one\n> + * to match this buffer with the correct video frame buffer.\n> + *\n> + * \\todo: Is there a better way to do this? What is the root cause?\n\nThis is odd to me. Both streams should start at 0 ... as they both do\nthe same operation.\n\n\n> + */\n> + unsigned int mdSequence = buffer->metadata().sequence + frameStart_;\n> + int pos = buffer->cookie();\n> +\n> + Span memMeta = mappedMetadataBuffers_.at(pos).planes()[0];\n> + uvc_meta_buf *metaBuf = reinterpret_cast(memMeta.data());\n> +\n> + //Span memTime = mappedMetadataBuffers_.at(pos).planes()[0];\n> + //UVCTimingBuf * timeBuf = reinterpret_cast(&memTime.data()[sizeof(uvc_meta_buf)]);\n\nPatches shouldn't introduce dead-code.\n\n> +\n> + size_t UVCPayloadHeaderSize = sizeof(metaBuf->length) +\n> + sizeof(metaBuf->flags) + sizeof(UVCTimingBuf);\n> + if (metaBuf->length < UVCPayloadHeaderSize) {\n\nIf I understand correctly - this could be checked at the point the\nbuffers are allocated, and will always be true/ok from that point onwards,\nso we could remove a fail path from the buffer completion handlers?\n\n> + endCorruptedStream();\n> + return;\n> + }\n> +\n> + /*\n> + * Match a pending video buffer with this buffer's sequence. If\n> + * there is none available, put this timestamp information on the\n> + * queue. When the matching video buffer does come in, it will use\n> + * a timestamp from this metadata.\n> + */\n> + if (!pendingVideoBuffers_.empty()) {\n> + FrameBuffer *vidBuffer = pendingVideoBuffers_.front();\n> + unsigned int vidSequence = vidBuffer->metadata().sequence;\n> +\n> + if (vidSequence == mdSequence) {\n> + completeRequest(vidBuffer, static_cast(metaBuf->ns));\n> + pendingVideoBuffers_.pop();\n> + } else {\n> + endCorruptedStream();\n\nThe big worry here is going to be what happens when a frame gets dropped\n(for any reason, including libcamera not running fast enough, or the\napplication not queuing buffers fast enough) ... so we'll have to deal\nwith this in a much safer way than just giving up and closing the\nstreams.\n\nI suspect what would really be needed is to put both buffer completions\nonto separate queues, that then triggers the function that will sort\nthrough both queues. When it has a match, it would complete the request.\nIf it detects a drop or an offset, it would complete with an error or\nthe old timestamp or such.\n\nI wonder if the implementors of the metadata video nodes considered\nthese cases...\n\n> + }\n> + } else {\n> + pendingMetadata_.push(\n> + std::make_pair(buffer->metadata().sequence,\n> + static_cast(metaBuf->ns)));\n> + }\n> + metadata_->queueBuffer(buffer);\n> +}\n> +\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)\n> \n> } /* namespace libcamera */\n> -- \n> 2.34.1\n>","headers":{"Return-Path":"","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 51921BF415\n\tfor ;\n\tMon, 21 Aug 2023 21:56:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6111B627E0;\n\tMon, 21 Aug 2023 23:56:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED46E61E09\n\tfor ;\n\tMon, 21 Aug 2023 23:56:22 +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 0D6626D5;\n\tMon, 21 Aug 2023 23:55:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692654984;\n\tbh=RVKmAKdrW0Qhe5yxAxtHTKceIKP3JuWhgyyNF+WHd0I=;\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=QZNcorrfUmqJXXy70HbqKrBSLaOfQv613typ53nrnH7StA4gE0AifJ2EXvPloIUdI\n\txrPc73H/8FKpC9QhURFfjTxi+jDXnGj74rl3t7I5Lk8+ascDaueXlMYGWxHUmcIO5+\n\ta5azNLQQyNaegw7dVrbM5Ff03r1wkVU68nKKo8AKigFjhDmQ0ZCMYiuAskuVBUvtW0\n\t3eBXchlGTZS/RfoaDVr24f9Bpxv3NJSrWfsg5dBk2G4Li7xf3N1BOjYt97GslDPuyJ\n\t03hCZ4BvJ/S1QJ0xBGp1Sqw/r2npd6v52uFUauC6C6sAijqt4z3NMDbdyYFw9Mf6hT\n\t0bZ9kks/jYkyQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1692654906;\n\tbh=RVKmAKdrW0Qhe5yxAxtHTKceIKP3JuWhgyyNF+WHd0I=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Vpq17StfCMEyYrCjkUukCUlLyiYiF2YoW/vC3OkqG800iq57564sAg0+QPE69SrdZ\n\tClDJgxWkAJLzpzQ7JMaW12josPAICM3NxCqXrIEGNJRliFErFi+cwYGuhQAs39jbiQ\n\trKLhiK88zBHuxjlkVJjEfHsZMrplTNH5im+oTxmU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Vpq17Stf\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20230821131039.127370-6-gabbymg94@gmail.com>","References":"<20230821131039.127370-1-gabbymg94@gmail.com>\n\t<20230821131039.127370-6-gabbymg94@gmail.com>","To":"gabbymg94@gmail.com, libcamera-devel@lists.libcamera.org,\n\tvedantparanjape160201@gmail.com","Date":"Mon, 21 Aug 2023 22:56:20 +0100","Message-ID":"<169265498017.435850.9404456795184764029@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [RFC PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27703,"web_url":"https://patchwork.libcamera.org/comment/27703/","msgid":"<20230828215011.GJ17083@pendragon.ideasonboard.com>","date":"2023-08-28T21:50:11","subject":"Re: [libcamera-devel] [RFC PATCH v2 5/5] libcamera: pipeline:\n\tuvcvideo: Handle metadata stream","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Aug 21, 2023 at 10:56:20PM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Gabby George (2023-08-21 14:10:39)\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 be improved upon in the next patch. For now, use\n> > the driver-provided metadata timestamp.\n> > \n> > Signed-off-by: Gabby George \n> > ---\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 168 ++++++++++++++++++-\n> > 1 file changed, 162 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index c8d6633f..215435ec 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -12,6 +12,8 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > +\n> > #include \n> > #include \n> > \n> > @@ -34,6 +36,17 @@ namespace libcamera {\n> > \n> > LOG_DEFINE_CATEGORY(UVC)\n> > \n> > +/*\n> > + * The UVCH buffer contains an unsigned char array\n> > + * encoding UVC timing data that needs to be recast\n> > + * into usable data.\n\nYou can reflow this to 80 columns.\n\n> > + */\n> > +struct UVCTimingBuf {\n> > + __u32 pts;\n> > + __u32 stc;\n> > + __u16 sofDevice;\n> > +} __attribute__((packed));\n> > +\n> > class UVCCameraData : public Camera::Private\n> > {\n> > public:\n> > @@ -46,6 +59,8 @@ 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> > + void handleUnfinishedRequests();\n> > \n> > const std::string &id() const { return id_; }\n> > \n> > @@ -56,9 +71,16 @@ public:\n> > std::map mappedMetadataBuffers_;\n> > \n> > std::map> formats_;\n> > + std::queue pendingVideoBuffers_;\n> > + std::queue> pendingMetadata_;\n> > \n> > private:\n> > int initMetadata(MediaDevice *media);\n> > + void completeRequest(FrameBuffer *buffer, uint64_t timestamp);\n> > + void endCorruptedStream();\n> > +\n> > + const unsigned int frameStart_ = 1;\n> > + const unsigned int maxVidBuffersInQueue_ = 1;\n> > \n> > bool generateId();\n> > \n> > @@ -242,7 +264,7 @@ int PipelineHandlerUVC::cleanupMetadataBuffers(Camera *camera)\n> > UVCCameraData *data = cameraData(camera);\n> > \n> > if (data->metadata_)\n> > - data->metadata_->releaseBuffers();\n> > + ret = data->metadata_->releaseBuffers();\n> \n> I don't think this change is specific to this patch, so it shouldn't be\n> here, but I don't think it would squash to a previous patch anyway as I\n> already mentioned the cleanup functions may as well be void cleanup();\n> \n> > data->metadataBuffers_.clear();\n> > data->mappedMetadataBuffers_.clear();\n> > data->metadata_ = nullptr;\n> > @@ -253,7 +275,9 @@ int PipelineHandlerUVC::cleanupMetadataBuffers(Camera *camera)\n> > int PipelineHandlerUVC::cleanup(Camera *camera)\n> > {\n> > UVCCameraData *data = cameraData(camera);\n> > +\n> > Y cleanupMetadataBuffers(camera);\n> > +\n\nThis should be squashed with one of the previous patches.\n\n> > data->video_->releaseBuffers();\n> > return 0;\n> > }\n> > @@ -354,6 +378,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n> > if (data->metadata_)\n> > data->metadata_->streamOff();\n> > \n> > + data->handleUnfinishedRequests();\n> > +\n> > cleanup(camera);\n> > }\n> > \n> > @@ -646,8 +672,11 @@ int UVCCameraData::init(MediaDevice *media)\n> > if (ret) {\n> > metadata_ = nullptr;\n> > LOG(UVC, Error) << \"Could not find a metadata video device.\";\n> > + return 0;\n\nThis is also unrelated to this patch.\n\n> > }\n> > \n> > + metadata_->bufferReady.connect(this, &UVCCameraData::bufferReadyMetadata);\n> > +\n> > return 0;\n> > }\n> > \n> > @@ -833,18 +862,145 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> > ctrls->emplace(id, info);\n> > }\n> > \n> > -void UVCCameraData::bufferReady(FrameBuffer *buffer)\n> > +void UVCCameraData::completeRequest(FrameBuffer *buffer, uint64_t timestamp)\n> > {\n> > Request *request = buffer->request();\n> > -\n> > - /* \\todo Use the UVC metadata to calculate a more precise timestamp */\n> > - request->metadata().set(controls::SensorTimestamp,\n> > - buffer->metadata().timestamp);\n> > + request->metadata().set(controls::SensorTimestamp, timestamp);\n> > \n> > pipe()->completeBuffer(request, buffer);\n> > pipe()->completeRequest(request);\n> > }\n> > \n> > +void UVCCameraData::handleUnfinishedRequests()\n> > +{\n> > + while (!pendingVideoBuffers_.empty()) {\n> > + FrameBuffer *oldBuffer = pendingVideoBuffers_.front();\n> > + Request *oldRequest = oldBuffer->request();\n> > +\n> > + oldRequest->metadata().set(controls::SensorTimestamp,\n> > + oldBuffer->metadata().timestamp);\n> > +\n> > + pipe()->completeBuffer(oldRequest, oldBuffer);\n> > + pipe()->completeRequest(oldRequest);\n> \n> Should this just call\n> \tcompleteRequest(oldBuffer, oldBuffer->metadata().timestamp); ?\n> \n> > + pendingVideoBuffers_.pop();\n> > + }\n> > +}\n> > +\n> > +void UVCCameraData::endCorruptedStream()\n> > +{\n> > + handleUnfinishedRequests();\n> > + /* Close the metadata node so we don't get inaccurate timestamps*/\n> > + metadata_ = nullptr;\n> > + LOG(UVC, Error)\n> > + << \"UVC metadata stream corrupted. Reverting to driver timestamps.\";\n> \n> Aha - so actually this doesn't stop the whole camera streaming - it just\n> stops the metadata node?\n> \n> Before closing the metadata_ should it release buffers and clean up\n> those ?\n> \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> > + /* \\todo Use the UVC metadata to calculate a more precise timestamp */\n> > + if (!metadata_ || buffer->metadata().sequence < frameStart_) {\n> > + completeRequest(buffer, buffer->metadata().timestamp);\n> > + return;\n> > + }\n> > +\n> > + if (!pendingMetadata_.empty()) {\n> > + /* A metadata buffer was ready first. */\n> > + unsigned int mdSequence = std::get<0>(pendingMetadata_.front()) + frameStart_;\n> > + if (mdSequence == buffer->metadata().sequence) {\n> > + completeRequest(buffer, std::get<1>(pendingMetadata_.front()));\n> > + pendingMetadata_.pop();\n> > + return;\n> > + } else {\n> > + /* \\todo: Is there a reason metadata buffers can arrive out of order? */\n> > + endCorruptedStream();\n> > + return;\n> > + }\n> > + }\n> > +\n> > + pendingVideoBuffers_.push(buffer);\n> > + /*\n> > + * Deal with video buffers that haven't been completed, and with\n> > + * buffers whose metadata information arrived out of order.\n> > + */\n> > + if (pendingVideoBuffers_.size() > maxVidBuffersInQueue_) {\n> > + endCorruptedStream();\n> \n> I'm worried about all cases this patch now introduces that seem to now\n> mean we'll abort a running camera stream. I wonder if that's just part\n> of the complexity introduced by having metadata delivered in multiple\n> video nodes though.\n> \n> > + }\n> > +}\n> > +\n> > +void UVCCameraData::bufferReadyMetadata(FrameBuffer *buffer)\n> > +{\n> > + if (!metadata_ ||\n> > + buffer->metadata().status != FrameMetadata::Status::FrameSuccess) {\n> > + return;\n> > + }\n> > +\n> > + /*\n> > + * The metadata stream seems to start at seq 1 and libcamera\n> > + * sets the start sequence to 0, so it's necessary to add one\n> > + * to match this buffer with the correct video frame buffer.\n> > + *\n> > + * \\todo: Is there a better way to do this? What is the root cause?\n> \n> This is odd to me. Both streams should start at 0 ... as they both do\n> the same operation.\n\nMaybe this is caused by queuing buffers for the metadata stream too late\n?\n\n> > + */\n> > + unsigned int mdSequence = buffer->metadata().sequence + frameStart_;\n> > + int pos = buffer->cookie();\n> > +\n> > + Span memMeta = mappedMetadataBuffers_.at(pos).planes()[0];\n> > + uvc_meta_buf *metaBuf = reinterpret_cast(memMeta.data());\n> > +\n> > + //Span memTime = mappedMetadataBuffers_.at(pos).planes()[0];\n> > + //UVCTimingBuf * timeBuf = reinterpret_cast(&memTime.data()[sizeof(uvc_meta_buf)]);\n> \n> Patches shouldn't introduce dead-code.\n> \n> > +\n> > + size_t UVCPayloadHeaderSize = sizeof(metaBuf->length) +\n> > + sizeof(metaBuf->flags) + sizeof(UVCTimingBuf);\n> > + if (metaBuf->length < UVCPayloadHeaderSize) {\n> \n> If I understand correctly - this could be checked at the point the\n> buffers are allocated, and will always be true/ok from that point onwards,\n> so we could remove a fail path from the buffer completion handlers?\n> \n> > + endCorruptedStream();\n> > + return;\n> > + }\n> > +\n> > + /*\n> > + * Match a pending video buffer with this buffer's sequence. If\n> > + * there is none available, put this timestamp information on the\n> > + * queue. When the matching video buffer does come in, it will use\n> > + * a timestamp from this metadata.\n> > + */\n> > + if (!pendingVideoBuffers_.empty()) {\n> > + FrameBuffer *vidBuffer = pendingVideoBuffers_.front();\n> > + unsigned int vidSequence = vidBuffer->metadata().sequence;\n> > +\n> > + if (vidSequence == mdSequence) {\n> > + completeRequest(vidBuffer, static_cast(metaBuf->ns));\n> > + pendingVideoBuffers_.pop();\n> > + } else {\n> > + endCorruptedStream();\n> \n> The big worry here is going to be what happens when a frame gets dropped\n> (for any reason, including libcamera not running fast enough, or the\n> application not queuing buffers fast enough) ... so we'll have to deal\n> with this in a much safer way than just giving up and closing the\n> streams.\n\nDefinitely :-)\n\n> I suspect what would really be needed is to put both buffer completions\n> onto separate queues, that then triggers the function that will sort\n> through both queues. When it has a match, it would complete the request.\n> If it detects a drop or an offset, it would complete with an error or\n> the old timestamp or such.\n> \n> I wonder if the implementors of the metadata video nodes considered\n> these cases...\n\nI'm not sure what needs to be considered on the kernel side. The above\nis how it should be handled in userspace.\n\n> > + }\n> > + } else {\n> > + pendingMetadata_.push(\n> > + std::make_pair(buffer->metadata().sequence,\n> > + static_cast(metaBuf->ns)));\n> > + }\n> > + metadata_->queueBuffer(buffer);\n> > +}\n> > +\n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerUVC)\n> > \n> > } /* namespace libcamera */","headers":{"Return-Path":"","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 90BD7C0F1B\n\tfor ;\n\tMon, 28 Aug 2023 21:50:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49E96627E1;\n\tMon, 28 Aug 2023 23:50:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C83F61F18\n\tfor ;\n\tMon, 28 Aug 2023 23:50:02 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 66257AF2;\n\tMon, 28 Aug 2023 23:48:40 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693259403;\n\tbh=Gb8Sf4Izu2aKWYLvtXp0w68ul9R3puDjYzXuCA6qDPo=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=HjCjWchy8Y2Nhd5JAukcE55/mnUbly8AWBwoczhC1BjBIC68b6ONlg7t1rwEzLJmN\n\tKIqVLdo6Ix2gJcJL+vVOfO4gijAWE3UtCEZlZlgEQmqlBCZMqSYHdV59JaztwUpKa1\n\tvdiGLhKF1eiK2c9jmmTx5n33GzoQqVAdKnYoD+boPb52myur5xVQE84kqyDD9htxf9\n\tyBKSZCQg5v9bSuH8IvGadV1pTp9jWgotQNg5Jh6LwULLItMISLl11OrKqf4kMyv5Tu\n\tjjDmp/nnmXgzEd1SHh3W1s9p/mSWlSVF8XU0pocVIU2/YSdbzg0KOAKvoRoui+2KiY\n\th8EY4Hck8oZHw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693259320;\n\tbh=Gb8Sf4Izu2aKWYLvtXp0w68ul9R3puDjYzXuCA6qDPo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fG/T2fKS1aS9QJhJKb3RG2524rL6mooPNfymCANz0wbpvp7dPyHQSiKb4xToE5LP/\n\tC8m5E53ZSIu8UjnRRiVVKrWERo9jXwfIrxd4/8R7yc4lL026COO8LZpEQQc/Lec3GB\n\tca5pO5jfilXIIsReIcvyrhj5yWwMRZmpcsW9w/T0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"fG/T2fKS\"; dkim-atps=neutral","Date":"Tue, 29 Aug 2023 00:50:11 +0300","To":"Kieran Bingham ","Message-ID":"<20230828215011.GJ17083@pendragon.ideasonboard.com>","References":"<20230821131039.127370-1-gabbymg94@gmail.com>\n\t<20230821131039.127370-6-gabbymg94@gmail.com>\n\t<169265498017.435850.9404456795184764029@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<169265498017.435850.9404456795184764029@ping.linuxembedded.co.uk>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]