[{"id":21619,"web_url":"https://patchwork.libcamera.org/comment/21619/","msgid":"","date":"2021-12-07T00:09:36","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:\n> The SensorTimestamp is defined to be the time of capture of the image.\n> While all streams should have the same timestamp, this is not always\n> defined or guaranteed as ISP drivers may not forward sequence numbers\n> and timestamps from their input buffer.\n\nThat should then bo solved by the pipeline handler, which should store\nthe correct timestamp in the buffer metadata.\n\n> Use the Request metadata to get the SensorTimestamp which must be\n> set by the pipeline handlers according to the data from the capture\n> device.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/cam/camera_session.cpp | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index 1bf460fa3fb7..50170723c30f 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request *request)\n> \tconst Request::BufferMap &buffers = request->buffers();\n> \n> \t/*\n> -\t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> -\t * the first buffer, as all buffers should have matching timestamps.\n> +\t * Compute the frame rate. The timestamp is retrieved from the\n> +\t * SensorTimestamp property, though all streams should have the\n> +\t * same timestamp.\n> \t */\n> -\tuint64_t ts = buffers.begin()->second->metadata().timestamp;\n> +\tuint64_t ts = request->metadata().get(controls::SensorTimestamp);\n\nThis seems reasonable. Why do we have timestamps in the buffer metadata\n? :-)\n\n> \tdouble fps = ts - last_;\n> \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> \tlast_ = ts;","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 32C49BF415\n\tfor ;\n\tTue, 7 Dec 2021 00:10:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 853F36087A;\n\tTue, 7 Dec 2021 01:10:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65EC460117\n\tfor ;\n\tTue, 7 Dec 2021 01:10:04 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E964E556;\n\tTue, 7 Dec 2021 01:10:03 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"r3lFX8qP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638835804;\n\tbh=Ul4XANQpGCLt3VY0VwsnXK5lYxKQasqcd9SQYeaaBjQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r3lFX8qPuhmSb5EvQnpst/WGmpVJl7F6ZqOoK2UQ1nxWuuLSKxLAzcr1OKsRgxu9d\n\tKWwCY8Gg4fuXNr8F7TTpSpx8kodcaLy6GZAoVEwGv7F4iFj7PB+c27X9phOJWECAvV\n\tiuN9S+In1/VvlRfxdg7KX2w/ihkrVRhZ4N3P4FsM=","Date":"Tue, 7 Dec 2021 02:09:36 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21627,"web_url":"https://patchwork.libcamera.org/comment/21627/","msgid":"<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>","date":"2021-12-07T13:21:09","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 12/7/21 5:39 AM, Laurent Pinchart wrote:\n> Hi Kieran,\n>\n> Thank you for the patch.\n>\n> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:\n>> The SensorTimestamp is defined to be the time of capture of the image.\n>> While all streams should have the same timestamp, this is not always\n>> defined or guaranteed as ISP drivers may not forward sequence numbers\n>> and timestamps from their input buffer.\n> That should then bo solved by the pipeline handler, which should store\n> the correct timestamp in the buffer metadata.\n>\n>> Use the Request metadata to get the SensorTimestamp which must be\n>> set by the pipeline handlers according to the data from the capture\n>> device.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/cam/camera_session.cpp | 7 ++++---\n>> 1 file changed, 4 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n>> index 1bf460fa3fb7..50170723c30f 100644\n>> --- a/src/cam/camera_session.cpp\n>> +++ b/src/cam/camera_session.cpp\n>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request *request)\n>> \tconst Request::BufferMap &buffers = request->buffers();\n>> \n>> \t/*\n>> -\t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n>> -\t * the first buffer, as all buffers should have matching timestamps.\n>> +\t * Compute the frame rate. The timestamp is retrieved from the\n>> +\t * SensorTimestamp property, though all streams should have the\n>> +\t * same timestamp.\n>> \t */\n>> -\tuint64_t ts = buffers.begin()->second->metadata().timestamp;\n>> +\tuint64_t ts = request->metadata().get(controls::SensorTimestamp);\n> This seems reasonable. Why do we have timestamps in the buffer metadata\n> ? :-)\n\n\nBecause there is no buffer assigned on the time-point where we want to \ncapture the timestamp.\n\nThe exact location of the timestamp is a \\todo\n\n              * \\todo The sensor timestamp should be better estimated by \nconnecting\n              * to the V4L2Device::frameStart signal.\n\nin all the pipeline-handlers as of now.\n\nWe *want* to capture at frameStart signal, which emits in response to \nVIDIOC_DQEVENT, but we can't as no buffer assigned (yet)\n\nWe probably need to assign a container taking care of timestamps with \nsequence numbers until a buffer is assigned down-the-line and then set \nthe buffer metadata by reading seq# & timestamp from that container.\n\n\n>\n>> \tdouble fps = ts - last_;\n>> \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n>> \tlast_ = ts;","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 80BE7BDB13\n\tfor ;\n\tTue, 7 Dec 2021 13:21:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5D7A605C4;\n\tTue, 7 Dec 2021 14:21:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FB9B60592\n\tfor ;\n\tTue, 7 Dec 2021 14:21:18 +0100 (CET)","from [IPv6:2401:4900:1f3e:16f1:4e1b:f869:54d4:8c0c] (unknown\n\t[IPv6:2401:4900:1f3e:16f1:4e1b:f869:54d4:8c0c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3FE46556;\n\tTue, 7 Dec 2021 14:21:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EvgwPnnM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638883278;\n\tbh=GdvqSj141T/c4/tmD0JPm9xf26VZrdk96/OOeQlRFuA=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=EvgwPnnM207O3hYaYJTNLGFK/qhsj3MuVe+JKNzuWkMdK65ODmcXOQRnMPWcWWINc\n\tslYjbEzl0RMAfebOFMERfPTmcdy/xCw9JmQ0G7nKPPNSoZX3J4VQZXnq/RGF0vBG0Z\n\tTgEMPxiKRlnxo79NTatFKA43lL5fWZHed2MaeyPc=","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>","Date":"Tue, 7 Dec 2021 18:51:09 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21634,"web_url":"https://patchwork.libcamera.org/comment/21634/","msgid":"<5622f398-4a0e-3abd-24fe-ede344d478fe@ideasonboard.com>","date":"2021-12-07T13:37:46","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"On 12/7/21 6:51 PM, Umang Jain wrote:\n> Hi Laurent,\n>\n> On 12/7/21 5:39 AM, Laurent Pinchart wrote:\n>> Hi Kieran,\n>>\n>> Thank you for the patch.\n>>\n>> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:\n>>> The SensorTimestamp is defined to be the time of capture of the image.\n>>> While all streams should have the same timestamp, this is not always\n>>> defined or guaranteed as ISP drivers may not forward sequence numbers\n>>> and timestamps from their input buffer.\n>> That should then bo solved by the pipeline handler, which should store\n>> the correct timestamp in the buffer metadata.\n>>\n>>> Use the Request metadata to get the SensorTimestamp which must be\n>>> set by the pipeline handlers according to the data from the capture\n>>> device.\n>>>\n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>>   src/cam/camera_session.cpp | 7 ++++---\n>>>   1 file changed, 4 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n>>> index 1bf460fa3fb7..50170723c30f 100644\n>>> --- a/src/cam/camera_session.cpp\n>>> +++ b/src/cam/camera_session.cpp\n>>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request \n>>> *request)\n>>>       const Request::BufferMap &buffers = request->buffers();\n>>>         /*\n>>> -     * Compute the frame rate. The timestamp is arbitrarily \n>>> retrieved from\n>>> -     * the first buffer, as all buffers should have matching \n>>> timestamps.\n>>> +     * Compute the frame rate. The timestamp is retrieved from the\n>>> +     * SensorTimestamp property, though all streams should have the\n>>> +     * same timestamp.\n>>>        */\n>>> -    uint64_t ts = buffers.begin()->second->metadata().timestamp;\n>>> +    uint64_t ts = request->metadata().get(controls::SensorTimestamp);\n>> This seems reasonable. Why do we have timestamps in the buffer metadata\n>> ? :-)\n\n\nStrong chance I have mis-understood the question, I later realized this \nis a cam-related patch.\n\nto me, the question translated :\n\n     why do we have timestamps in the FrameBuffer.metadata_.timestamp ?\n\nSo ignore the discussion (if you want) :-P\n\n>\n>\n> Because there is no buffer assigned on the time-point where we want to \n> capture the timestamp.\n>\n> The exact location of the timestamp is a \\todo\n>\n>              * \\todo The sensor timestamp should be better estimated \n> by connecting\n>              * to the V4L2Device::frameStart signal.\n>\n> in all the pipeline-handlers as of now.\n>\n> We *want* to capture at frameStart signal, which emits in response to \n> VIDIOC_DQEVENT, but we can't as no buffer assigned (yet)\n>\n> We probably need to assign a container taking care of timestamps with \n> sequence numbers until a buffer is assigned down-the-line and then set \n> the buffer metadata by reading seq# & timestamp from that container.\n>\n>\n>>\n>>>       double fps = ts - last_;\n>>>       fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n>>>       last_ = ts;","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 7B883BF415\n\tfor ;\n\tTue, 7 Dec 2021 13:37:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30FDA6086A;\n\tTue, 7 Dec 2021 14:37:56 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ECBB260592\n\tfor ;\n\tTue, 7 Dec 2021 14:37:54 +0100 (CET)","from [IPv6:2401:4900:1f3e:16f1:4e1b:f869:54d4:8c0c] (unknown\n\t[IPv6:2401:4900:1f3e:16f1:4e1b:f869:54d4:8c0c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 38EAE556;\n\tTue, 7 Dec 2021 14:37:52 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wZQMWgdP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638884274;\n\tbh=XBEm6VS6o+NT5cIn0arDIbpTTWRBD01+GR+s5GyECpY=;\n\th=Subject:From:To:Cc:References:Date:In-Reply-To:From;\n\tb=wZQMWgdPP+f06rqW7HIgDjXv1RQSGSpyLjUtj104KeFbUbTk0QdWZfQg4yJnRYJaF\n\talXhLo6pmL8BWETdjUP6a9JMCa5H5hLnccO6/2jb/o5SS7BdpGbfNI58BR9saorAJh\n\tzlPv4F3eqZ2kdSGTTXQ3t4PBL4neoQUTHWPWvigg=","From":"Umang Jain ","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>\n\t\n\t<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>","Message-ID":"<5622f398-4a0e-3abd-24fe-ede344d478fe@ideasonboard.com>","Date":"Tue, 7 Dec 2021 19:07:46 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21679,"web_url":"https://patchwork.libcamera.org/comment/21679/","msgid":"","date":"2021-12-08T18:57:10","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Tue, Dec 07, 2021 at 07:07:46PM +0530, Umang Jain wrote:\n> On 12/7/21 6:51 PM, Umang Jain wrote:\n> > On 12/7/21 5:39 AM, Laurent Pinchart wrote:\n> >> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:\n> >>> The SensorTimestamp is defined to be the time of capture of the image.\n> >>> While all streams should have the same timestamp, this is not always\n> >>> defined or guaranteed as ISP drivers may not forward sequence numbers\n> >>> and timestamps from their input buffer.\n> >>\n> >> That should then bo solved by the pipeline handler, which should store\n> >> the correct timestamp in the buffer metadata.\n> >>\n> >>> Use the Request metadata to get the SensorTimestamp which must be\n> >>> set by the pipeline handlers according to the data from the capture\n> >>> device.\n> >>>\n> >>> Signed-off-by: Kieran Bingham \n> >>> ---\n> >>>   src/cam/camera_session.cpp | 7 ++++---\n> >>>   1 file changed, 4 insertions(+), 3 deletions(-)\n> >>>\n> >>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> >>> index 1bf460fa3fb7..50170723c30f 100644\n> >>> --- a/src/cam/camera_session.cpp\n> >>> +++ b/src/cam/camera_session.cpp\n> >>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request \n> >>> *request)\n> >>>       const Request::BufferMap &buffers = request->buffers();\n> >>>         /*\n> >>> -     * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> >>> -     * the first buffer, as all buffers should have matching timestamps.\n> >>> +     * Compute the frame rate. The timestamp is retrieved from the\n> >>> +     * SensorTimestamp property, though all streams should have the\n> >>> +     * same timestamp.\n> >>>        */\n> >>> -    uint64_t ts = buffers.begin()->second->metadata().timestamp;\n> >>> +    uint64_t ts = request->metadata().get(controls::SensorTimestamp);\n> >>\n> >> This seems reasonable. Why do we have timestamps in the buffer metadata\n> >> ? :-)\n> \n> Strong chance I have mis-understood the question, I later realized this \n> is a cam-related patch.\n> \n> to me, the question translated :\n> \n>     why do we have timestamps in the FrameBuffer.metadata_.timestamp ?\n\nTo clarify, I meant to ask why we have both controls::SensorTimestamp\n*and* timestamps in individual buffers\n(FrameBuffer::metadata_.timestamp). I suppose it's historical, the\ntimestamp member comes from\n\ncommit e94e52c0cb27f92c085da6e776af8b3d3172bbb2\nAuthor: Niklas Söderlund \nDate: Thu Jan 24 23:34:51 2019 +0100\n\n libcamera: v4l2_device: Update dequeued buffer information\n\nwhile controls::SensorTimestamp has been introduced in\n\ncommit 84e79bd8b5347d91309cbd759bbd988b5144fb8d\nAuthor: Jacopo Mondi \nDate: Thu Jul 30 15:18:50 2020 +0200\n\n libcamera: control_ids: Define draft controls\n\n> So ignore the discussion (if you want) :-P\n> \n> > Because there is no buffer assigned on the time-point where we want to \n> > capture the timestamp.\n> >\n> > The exact location of the timestamp is a \\todo\n> >\n> >              * \\todo The sensor timestamp should be better estimated \n> > by connecting\n> >              * to the V4L2Device::frameStart signal.\n> >\n> > in all the pipeline-handlers as of now.\n> >\n> > We *want* to capture at frameStart signal, which emits in response to \n> > VIDIOC_DQEVENT, but we can't as no buffer assigned (yet)\n> >\n> > We probably need to assign a container taking care of timestamps with \n> > sequence numbers until a buffer is assigned down-the-line and then set \n> > the buffer metadata by reading seq# & timestamp from that container.\n\nShould we just drop FrameBuffer::metadata_.timestamp in favour of\ncontrols::SensorTimestamp ? What would be the drawbacks, if any ?\n\n> >>>       double fps = ts - last_;\n> >>>       fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> >>>       last_ = ts;","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 6FA82BF415\n\tfor ;\n\tWed, 8 Dec 2021 18:57:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C755C60725;\n\tWed, 8 Dec 2021 19:57:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6DB5260225\n\tfor ;\n\tWed, 8 Dec 2021 19:57:40 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D1F3CB6C;\n\tWed, 8 Dec 2021 19:57:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eC3DUNu5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638989860;\n\tbh=cV/sui5hjkiv9SdjvV6aR/kAYRj4KgRBQSfUxWz/wdo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eC3DUNu5F1lZLRmvc14vp5dWNn1lEugpXFnjH314Pzt79cWdofOgi4BsGXJQycoOi\n\t9GwTsQMf1JNufOgRGnINiBi25x/91jReqtA/FSLo0USt+6Qi92hsP0vxEVK/llykED\n\tdr4TiYpcJVEMx5PYxWkvXFGMDFQiWtD+K+0IddEY=","Date":"Wed, 8 Dec 2021 20:57:10 +0200","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>\n\t\n\t<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>\n\t<5622f398-4a0e-3abd-24fe-ede344d478fe@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<5622f398-4a0e-3abd-24fe-ede344d478fe@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21680,"web_url":"https://patchwork.libcamera.org/comment/21680/","msgid":"<163899716920.2066819.5739362313782502963@Monstersaurus>","date":"2021-12-08T20:59:29","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-12-08 18:57:10)\n> Hi Umang,\n> \n> On Tue, Dec 07, 2021 at 07:07:46PM +0530, Umang Jain wrote:\n> > On 12/7/21 6:51 PM, Umang Jain wrote:\n> > > On 12/7/21 5:39 AM, Laurent Pinchart wrote:\n> > >> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:\n> > >>> The SensorTimestamp is defined to be the time of capture of the image.\n> > >>> While all streams should have the same timestamp, this is not always\n> > >>> defined or guaranteed as ISP drivers may not forward sequence numbers\n> > >>> and timestamps from their input buffer.\n> > >>\n> > >> That should then bo solved by the pipeline handler, which should store\n> > >> the correct timestamp in the buffer metadata.\n> > >>\n> > >>> Use the Request metadata to get the SensorTimestamp which must be\n> > >>> set by the pipeline handlers according to the data from the capture\n> > >>> device.\n> > >>>\n> > >>> Signed-off-by: Kieran Bingham \n> > >>> ---\n> > >>>   src/cam/camera_session.cpp | 7 ++++---\n> > >>>   1 file changed, 4 insertions(+), 3 deletions(-)\n> > >>>\n> > >>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > >>> index 1bf460fa3fb7..50170723c30f 100644\n> > >>> --- a/src/cam/camera_session.cpp\n> > >>> +++ b/src/cam/camera_session.cpp\n> > >>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request \n> > >>> *request)\n> > >>>       const Request::BufferMap &buffers = request->buffers();\n> > >>>         /*\n> > >>> -     * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> > >>> -     * the first buffer, as all buffers should have matching timestamps.\n> > >>> +     * Compute the frame rate. The timestamp is retrieved from the\n> > >>> +     * SensorTimestamp property, though all streams should have the\n> > >>> +     * same timestamp.\n> > >>>        */\n> > >>> -    uint64_t ts = buffers.begin()->second->metadata().timestamp;\n> > >>> +    uint64_t ts = request->metadata().get(controls::SensorTimestamp);\n> > >>\n> > >> This seems reasonable. Why do we have timestamps in the buffer metadata\n> > >> ? :-)\n> > \n> > Strong chance I have mis-understood the question, I later realized this \n> > is a cam-related patch.\n> > \n> > to me, the question translated :\n> > \n> >     why do we have timestamps in the FrameBuffer.metadata_.timestamp ?\n> \n> To clarify, I meant to ask why we have both controls::SensorTimestamp\n> *and* timestamps in individual buffers\n> (FrameBuffer::metadata_.timestamp). I suppose it's historical, the\n> timestamp member comes from\n> \n> commit e94e52c0cb27f92c085da6e776af8b3d3172bbb2\n> Author: Niklas Söderlund \n> Date: Thu Jan 24 23:34:51 2019 +0100\n> \n> libcamera: v4l2_device: Update dequeued buffer information\n> \n> while controls::SensorTimestamp has been introduced in\n> \n> commit 84e79bd8b5347d91309cbd759bbd988b5144fb8d\n> Author: Jacopo Mondi \n> Date: Thu Jul 30 15:18:50 2020 +0200\n> \n> libcamera: control_ids: Define draft controls\n> \n> > So ignore the discussion (if you want) :-P\n> > \n> > > Because there is no buffer assigned on the time-point where we want to \n> > > capture the timestamp.\n> > >\n> > > The exact location of the timestamp is a \\todo\n> > >\n> > >              * \\todo The sensor timestamp should be better estimated \n> > > by connecting\n> > >              * to the V4L2Device::frameStart signal.\n> > >\n> > > in all the pipeline-handlers as of now.\n> > >\n> > > We *want* to capture at frameStart signal, which emits in response to \n> > > VIDIOC_DQEVENT, but we can't as no buffer assigned (yet)\n> > >\n> > > We probably need to assign a container taking care of timestamps with \n> > > sequence numbers until a buffer is assigned down-the-line and then set \n> > > the buffer metadata by reading seq# & timestamp from that container.\n> \n> Should we just drop FrameBuffer::metadata_.timestamp in favour of\n> controls::SensorTimestamp ? What would be the drawbacks, if any ?\n\nDrawbacks for using Metadata control:\n - Have to parse the metadata control list to get timestamps\n (/sequences)\n\nPro's for Metadata control:\n - Only one place for it, all buffers in the same request are defined as\n having the same value (sequence and timestamp).\n\n\nDrawbacks for per-buffer metadata:\n - Must be explictily assigned to keep them in sync even though they are\n the output of multiple video devices perhaps.\n\nPro's for for per-buffer metadata:\n - Quicker to retrieve the values? (defined structure?)\n\nAny more on either side?\n\n\nPersonally, I'm currently thinking these are better defined by the\nmetadata() control list. After all, that's the purpose of the list\nright?\n\n\n> \n> > >>>       double fps = ts - last_;\n> > >>>       fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> > >>>       last_ = ts;\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 ED60EBF415\n\tfor ;\n\tWed, 8 Dec 2021 20:59:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A6326086A;\n\tWed, 8 Dec 2021 21:59:34 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48D7960225\n\tfor ;\n\tWed, 8 Dec 2021 21:59:32 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B9DE1B6C;\n\tWed, 8 Dec 2021 21:59:31 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NHLJALOL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638997171;\n\tbh=+gfZl6fThidWevIwwJipY/lnG6D9vOtYFh5Y6YtYgEg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=NHLJALOLhRCSm4kJ6IehQhBLECwdNr1tHHNeIqrw8xjx3vsorqgW3SOre/0qwy9yn\n\tnQqYAJhegfHwuAEfXgveaYJO1xHysVP80HLI3n3G8n5KcUjjU6WhpuRP7NshtI84dj\n\tVRyWeED9li1i1xcz3ZF0tLxiX605xFt5l44oD5m8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>\n\t\n\t<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>\n\t<5622f398-4a0e-3abd-24fe-ede344d478fe@ideasonboard.com>\n\t","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tUmang Jain ","Date":"Wed, 08 Dec 2021 20:59:29 +0000","Message-ID":"<163899716920.2066819.5739362313782502963@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21681,"web_url":"https://patchwork.libcamera.org/comment/21681/","msgid":"","date":"2021-12-08T21:13:00","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Dec 08, 2021 at 08:59:29PM +0000, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2021-12-08 18:57:10)\n> > On Tue, Dec 07, 2021 at 07:07:46PM +0530, Umang Jain wrote:\n> > > On 12/7/21 6:51 PM, Umang Jain wrote:\n> > > > On 12/7/21 5:39 AM, Laurent Pinchart wrote:\n> > > >> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:\n> > > >>> The SensorTimestamp is defined to be the time of capture of the image.\n> > > >>> While all streams should have the same timestamp, this is not always\n> > > >>> defined or guaranteed as ISP drivers may not forward sequence numbers\n> > > >>> and timestamps from their input buffer.\n> > > >>\n> > > >> That should then bo solved by the pipeline handler, which should store\n> > > >> the correct timestamp in the buffer metadata.\n> > > >>\n> > > >>> Use the Request metadata to get the SensorTimestamp which must be\n> > > >>> set by the pipeline handlers according to the data from the capture\n> > > >>> device.\n> > > >>>\n> > > >>> Signed-off-by: Kieran Bingham \n> > > >>> ---\n> > > >>>   src/cam/camera_session.cpp | 7 ++++---\n> > > >>>   1 file changed, 4 insertions(+), 3 deletions(-)\n> > > >>>\n> > > >>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > > >>> index 1bf460fa3fb7..50170723c30f 100644\n> > > >>> --- a/src/cam/camera_session.cpp\n> > > >>> +++ b/src/cam/camera_session.cpp\n> > > >>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request \n> > > >>> *request)\n> > > >>>       const Request::BufferMap &buffers = request->buffers();\n> > > >>>         /*\n> > > >>> -     * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> > > >>> -     * the first buffer, as all buffers should have matching timestamps.\n> > > >>> +     * Compute the frame rate. The timestamp is retrieved from the\n> > > >>> +     * SensorTimestamp property, though all streams should have the\n> > > >>> +     * same timestamp.\n> > > >>>        */\n> > > >>> -    uint64_t ts = buffers.begin()->second->metadata().timestamp;\n> > > >>> +    uint64_t ts = request->metadata().get(controls::SensorTimestamp);\n> > > >>\n> > > >> This seems reasonable. Why do we have timestamps in the buffer metadata\n> > > >> ? :-)\n> > > \n> > > Strong chance I have mis-understood the question, I later realized this \n> > > is a cam-related patch.\n> > > \n> > > to me, the question translated :\n> > > \n> > >     why do we have timestamps in the FrameBuffer.metadata_.timestamp ?\n> > \n> > To clarify, I meant to ask why we have both controls::SensorTimestamp\n> > *and* timestamps in individual buffers\n> > (FrameBuffer::metadata_.timestamp). I suppose it's historical, the\n> > timestamp member comes from\n> > \n> > commit e94e52c0cb27f92c085da6e776af8b3d3172bbb2\n> > Author: Niklas Söderlund \n> > Date: Thu Jan 24 23:34:51 2019 +0100\n> > \n> > libcamera: v4l2_device: Update dequeued buffer information\n> > \n> > while controls::SensorTimestamp has been introduced in\n> > \n> > commit 84e79bd8b5347d91309cbd759bbd988b5144fb8d\n> > Author: Jacopo Mondi \n> > Date: Thu Jul 30 15:18:50 2020 +0200\n> > \n> > libcamera: control_ids: Define draft controls\n> > \n> > > So ignore the discussion (if you want) :-P\n> > > \n> > > > Because there is no buffer assigned on the time-point where we want to \n> > > > capture the timestamp.\n> > > >\n> > > > The exact location of the timestamp is a \\todo\n> > > >\n> > > >              * \\todo The sensor timestamp should be better estimated \n> > > > by connecting\n> > > >              * to the V4L2Device::frameStart signal.\n> > > >\n> > > > in all the pipeline-handlers as of now.\n> > > >\n> > > > We *want* to capture at frameStart signal, which emits in response to \n> > > > VIDIOC_DQEVENT, but we can't as no buffer assigned (yet)\n> > > >\n> > > > We probably need to assign a container taking care of timestamps with \n> > > > sequence numbers until a buffer is assigned down-the-line and then set \n> > > > the buffer metadata by reading seq# & timestamp from that container.\n> > \n> > Should we just drop FrameBuffer::metadata_.timestamp in favour of\n> > controls::SensorTimestamp ? What would be the drawbacks, if any ?\n> \n> Drawbacks for using Metadata control:\n> - Have to parse the metadata control list to get timestamps\n> (/sequences)\n\nThe request metadata is also not available before the request fully\ncompletes, while buffers will complete individually before. This is\nsomething we may want to address in a generic way though, I suspect\nthere will be use cases for providing request metadata early.\n\n> Pro's for Metadata control:\n> - Only one place for it, all buffers in the same request are defined as\n> having the same value (sequence and timestamp).\n\nThat's the part I like :-)\n\n> Drawbacks for per-buffer metadata:\n> - Must be explictily assigned to keep them in sync even though they are\n> the output of multiple video devices perhaps.\n> \n> Pro's for for per-buffer metadata:\n> - Quicker to retrieve the values? (defined structure?)\n\nPossibly a bit quicker, but that seems marginal to me.\n\n> Any more on either side?\n> \n> \n> Personally, I'm currently thinking these are better defined by the\n> metadata() control list. After all, that's the purpose of the list\n> right?\n\nI think so. I see very little value today in providing buffer completion\ntimestamps that would be different than the sensor timestamp (this could\npossibly help measuring the processing time, but if that's the goal, it\ncould also be done by capturing the system timestamp in the buffer\ncompletion handler on the application side, which would provide\nstatistics about the full processing time, including both the hardware\nprocessing and the software processing).\n\nLet's wait for more feedback, but if there's a general agreement that\nthis is the direction we want to take, we could remove\nFrameBuffer::metadata_.timestamp independently of addressing the sensor\nsequence issue.\n\n> > > >>>       double fps = ts - last_;\n> > > >>>       fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> > > >>>       last_ = ts;","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 92519BDB13\n\tfor ;\n\tWed, 8 Dec 2021 21:13:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E51006086A;\n\tWed, 8 Dec 2021 22:13:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93F1460225\n\tfor ;\n\tWed, 8 Dec 2021 22:13:30 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0DF8DB6C;\n\tWed, 8 Dec 2021 22:13:29 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VpJgQX6l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638998010;\n\tbh=us3mg/IG4y1cxO8Knqyx0v4nXe1FW3eRdu0Hc4YoFVY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VpJgQX6lcN2+iDUd56LVnIv8Ysf26pgogKOXUrlPK0k52H9uWF8gRarJo+ezRG0HN\n\tU/9DS4F2TZ1NowL1Gl3oHkJZTgEUeydy6qZDzBGCyqOiFT149Ay2h/1tgpumxEw290\n\t7JL6cQ3VbYYl1poJ7xTENSxNfA4Vf2a4yPAvwW/M=","Date":"Wed, 8 Dec 2021 23:13:00 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>\n\t\n\t<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>\n\t<5622f398-4a0e-3abd-24fe-ede344d478fe@ideasonboard.com>\n\t\n\t<163899716920.2066819.5739362313782502963@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<163899716920.2066819.5739362313782502963@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21683,"web_url":"https://patchwork.libcamera.org/comment/21683/","msgid":"<04707549-fb22-5ff8-a1f2-22b6b2ee7012@ideasonboard.com>","date":"2021-12-09T05:59:20","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nOn 12/9/21 2:43 AM, Laurent Pinchart wrote:\n> Hi Kieran,\n>\n> On Wed, Dec 08, 2021 at 08:59:29PM +0000, Kieran Bingham wrote:\n>> Quoting Laurent Pinchart (2021-12-08 18:57:10)\n>>> On Tue, Dec 07, 2021 at 07:07:46PM +0530, Umang Jain wrote:\n>>>> On 12/7/21 6:51 PM, Umang Jain wrote:\n>>>>> On 12/7/21 5:39 AM, Laurent Pinchart wrote:\n>>>>>> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:\n>>>>>>> The SensorTimestamp is defined to be the time of capture of the image.\n>>>>>>> While all streams should have the same timestamp, this is not always\n>>>>>>> defined or guaranteed as ISP drivers may not forward sequence numbers\n>>>>>>> and timestamps from their input buffer.\n>>>>>> That should then bo solved by the pipeline handler, which should store\n>>>>>> the correct timestamp in the buffer metadata.\n>>>>>>\n>>>>>>> Use the Request metadata to get the SensorTimestamp which must be\n>>>>>>> set by the pipeline handlers according to the data from the capture\n>>>>>>> device.\n>>>>>>>\n>>>>>>> Signed-off-by: Kieran Bingham \n>>>>>>> ---\n>>>>>>>   src/cam/camera_session.cpp | 7 ++++---\n>>>>>>>   1 file changed, 4 insertions(+), 3 deletions(-)\n>>>>>>>\n>>>>>>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n>>>>>>> index 1bf460fa3fb7..50170723c30f 100644\n>>>>>>> --- a/src/cam/camera_session.cpp\n>>>>>>> +++ b/src/cam/camera_session.cpp\n>>>>>>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request\n>>>>>>> *request)\n>>>>>>>       const Request::BufferMap &buffers = request->buffers();\n>>>>>>>         /*\n>>>>>>> -     * Compute the frame rate. The timestamp is arbitrarily retrieved from\n>>>>>>> -     * the first buffer, as all buffers should have matching timestamps.\n>>>>>>> +     * Compute the frame rate. The timestamp is retrieved from the\n>>>>>>> +     * SensorTimestamp property, though all streams should have the\n>>>>>>> +     * same timestamp.\n>>>>>>>        */\n>>>>>>> -    uint64_t ts = buffers.begin()->second->metadata().timestamp;\n>>>>>>> +    uint64_t ts = request->metadata().get(controls::SensorTimestamp);\n>>>>>> This seems reasonable. Why do we have timestamps in the buffer metadata\n>>>>>> ? :-)\n>>>> Strong chance I have mis-understood the question, I later realized this\n>>>> is a cam-related patch.\n>>>>\n>>>> to me, the question translated :\n>>>>\n>>>>     why do we have timestamps in the FrameBuffer.metadata_.timestamp ?\n>>> To clarify, I meant to ask why we have both controls::SensorTimestamp\n>>> *and* timestamps in individual buffers\n>>> (FrameBuffer::metadata_.timestamp). I suppose it's historical, the\n>>> timestamp member comes from\n>>>\n>>> commit e94e52c0cb27f92c085da6e776af8b3d3172bbb2\n>>> Author: Niklas Söderlund \n>>> Date: Thu Jan 24 23:34:51 2019 +0100\n>>>\n>>> libcamera: v4l2_device: Update dequeued buffer information\n>>>\n>>> while controls::SensorTimestamp has been introduced in\n>>>\n>>> commit 84e79bd8b5347d91309cbd759bbd988b5144fb8d\n>>> Author: Jacopo Mondi \n>>> Date: Thu Jul 30 15:18:50 2020 +0200\n>>>\n>>> libcamera: control_ids: Define draft controls\n>>>\n>>>> So ignore the discussion (if you want) :-P\n>>>>\n>>>>> Because there is no buffer assigned on the time-point where we want to\n>>>>> capture the timestamp.\n>>>>>\n>>>>> The exact location of the timestamp is a \\todo\n>>>>>\n>>>>>              * \\todo The sensor timestamp should be better estimated\n>>>>> by connecting\n>>>>>              * to the V4L2Device::frameStart signal.\n>>>>>\n>>>>> in all the pipeline-handlers as of now.\n>>>>>\n>>>>> We *want* to capture at frameStart signal, which emits in response to\n>>>>> VIDIOC_DQEVENT, but we can't as no buffer assigned (yet)\n>>>>>\n>>>>> We probably need to assign a container taking care of timestamps with\n>>>>> sequence numbers until a buffer is assigned down-the-line and then set\n>>>>> the buffer metadata by reading seq# & timestamp from that container.\n>>> Should we just drop FrameBuffer::metadata_.timestamp in favour of\n>>> controls::SensorTimestamp ? What would be the drawbacks, if any ?\n>> Drawbacks for using Metadata control:\n>> - Have to parse the metadata control list to get timestamps\n>> (/sequences)\n> The request metadata is also not available before the request fully\n> completes, while buffers will complete individually before. This is\n> something we may want to address in a generic way though, I suspect\n> there will be use cases for providing request metadata early.\n>\n>> Pro's for Metadata control:\n>> - Only one place for it, all buffers in the same request are defined as\n>> having the same value (sequence and timestamp).\n> That's the part I like :-)\n>\n>> Drawbacks for per-buffer metadata:\n>> - Must be explictily assigned to keep them in sync even though they are\n>> the output of multiple video devices perhaps.\n>>\n>> Pro's for for per-buffer metadata:\n>> - Quicker to retrieve the values? (defined structure?)\n> Possibly a bit quicker, but that seems marginal to me.\n>\n>> Any more on either side?\n>>\n>>\n>> Personally, I'm currently thinking these are better defined by the\n>> metadata() control list. After all, that's the purpose of the list\n>> right?\n> I think so. I see very little value today in providing buffer completion\n> timestamps that would be different than the sensor timestamp (this could\n\n\nAgreed on this point. Applications 'generally' should be asking for \nrequest-completion timestamp and sensor-timestamp (if they want to use \nit for hw/processing metrics).\n\nBuffer-completion timestamps are in-between (also sub-classed facts like \n\"direct\" buffers / post-processed buffers etc.)  - I can't see any major \nvalue for applications for requesting those, but I could be wrong!\n\n> possibly help measuring the processing time, but if that's the goal, it\n> could also be done by capturing the system timestamp in the buffer\n> completion handler on the application side, which would provide\n> statistics about the full processing time, including both the hardware\n> processing and the software processing).\n>\n> Let's wait for more feedback, but if there's a general agreement that\n> this is the direction we want to take, we could remove\n> FrameBuffer::metadata_.timestamp independently of addressing the sensor\n> sequence issue.\n\n\nMakes sense.\n\n>\n>>>>>>>       double fps = ts - last_;\n>>>>>>>       fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n>>>>>>>       last_ = ts;","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 34CDEBDB13\n\tfor ;\n\tThu, 9 Dec 2021 05:59:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1909960822;\n\tThu, 9 Dec 2021 06:59:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 62AB960114\n\tfor ;\n\tThu, 9 Dec 2021 06:59:29 +0100 (CET)","from [IPv6:2401:4900:1f3e:37a3:270b:b541:a3a9:933b] (unknown\n\t[IPv6:2401:4900:1f3e:37a3:270b:b541:a3a9:933b])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 53A77501;\n\tThu, 9 Dec 2021 06:59:26 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"As68RmiE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639029568;\n\tbh=X711qNNXjYwrCJr5kfyplqCUusjegatl6Btsyhr2LQw=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=As68RmiEtStJ8RXQY6e4tqkqFJJ24JZnY9S+7TaPRK8ZtT5ucVLRosIlIFJCQHPyS\n\t+aziGPvYwaytW6TZYMBb/StxP2aO5aSSDJ/zAWxZeTkWvLaz6a/QiEl83M7TmKRatf\n\t63MJzT/zHX6Ugmerq2sUz/R3ANlZZXoSP+1Gb+5A=","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>\n\t\n\t<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>\n\t<5622f398-4a0e-3abd-24fe-ede344d478fe@ideasonboard.com>\n\t\n\t<163899716920.2066819.5739362313782502963@Monstersaurus>\n\t","From":"Umang Jain ","Message-ID":"<04707549-fb22-5ff8-a1f2-22b6b2ee7012@ideasonboard.com>","Date":"Thu, 9 Dec 2021 11:29:20 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21687,"web_url":"https://patchwork.libcamera.org/comment/21687/","msgid":"","date":"2021-12-09T09:23:03","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\nOn Wed, 8 Dec 2021 at 21:13, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Kieran,\n>\n> On Wed, Dec 08, 2021 at 08:59:29PM +0000, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2021-12-08 18:57:10)\n> > > On Tue, Dec 07, 2021 at 07:07:46PM +0530, Umang Jain wrote:\n> > > > On 12/7/21 6:51 PM, Umang Jain wrote:\n> > > > > On 12/7/21 5:39 AM, Laurent Pinchart wrote:\n> > > > >> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:\n> > > > >>> The SensorTimestamp is defined to be the time of capture of the\n> image.\n> > > > >>> While all streams should have the same timestamp, this is not\n> always\n> > > > >>> defined or guaranteed as ISP drivers may not forward sequence\n> numbers\n> > > > >>> and timestamps from their input buffer.\n> > > > >>\n> > > > >> That should then bo solved by the pipeline handler, which should\n> store\n> > > > >> the correct timestamp in the buffer metadata.\n> > > > >>\n> > > > >>> Use the Request metadata to get the SensorTimestamp which must be\n> > > > >>> set by the pipeline handlers according to the data from the\n> capture\n> > > > >>> device.\n> > > > >>>\n> > > > >>> Signed-off-by: Kieran Bingham \n> > > > >>> ---\n> > > > >>> src/cam/camera_session.cpp | 7 ++++---\n> > > > >>> 1 file changed, 4 insertions(+), 3 deletions(-)\n> > > > >>>\n> > > > >>> diff --git a/src/cam/camera_session.cpp\n> b/src/cam/camera_session.cpp\n> > > > >>> index 1bf460fa3fb7..50170723c30f 100644\n> > > > >>> --- a/src/cam/camera_session.cpp\n> > > > >>> +++ b/src/cam/camera_session.cpp\n> > > > >>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request\n> > > > >>> *request)\n> > > > >>> const Request::BufferMap &buffers = request->buffers();\n> > > > >>> /*\n> > > > >>> - * Compute the frame rate. The timestamp is arbitrarily\n> retrieved from\n> > > > >>> - * the first buffer, as all buffers should have matching\n> timestamps.\n> > > > >>> + * Compute the frame rate. The timestamp is retrieved from\n> the\n> > > > >>> + * SensorTimestamp property, though all streams should have\n> the\n> > > > >>> + * same timestamp.\n> > > > >>> */\n> > > > >>> - uint64_t ts = buffers.begin()->second->metadata().timestamp;\n> > > > >>> + uint64_t ts =\n> request->metadata().get(controls::SensorTimestamp);\n> > > > >>\n> > > > >> This seems reasonable. Why do we have timestamps in the buffer\n> metadata\n> > > > >> ? :-)\n> > > >\n> > > > Strong chance I have mis-understood the question, I later realized\n> this\n> > > > is a cam-related patch.\n> > > >\n> > > > to me, the question translated :\n> > > >\n> > > > why do we have timestamps in the\n> FrameBuffer.metadata_.timestamp ?\n> > >\n> > > To clarify, I meant to ask why we have both controls::SensorTimestamp\n> > > *and* timestamps in individual buffers\n> > > (FrameBuffer::metadata_.timestamp). I suppose it's historical, the\n> > > timestamp member comes from\n> > >\n> > > commit e94e52c0cb27f92c085da6e776af8b3d3172bbb2\n> > > Author: Niklas Söderlund \n> > > Date: Thu Jan 24 23:34:51 2019 +0100\n> > >\n> > > libcamera: v4l2_device: Update dequeued buffer information\n> > >\n> > > while controls::SensorTimestamp has been introduced in\n> > >\n> > > commit 84e79bd8b5347d91309cbd759bbd988b5144fb8d\n> > > Author: Jacopo Mondi \n> > > Date: Thu Jul 30 15:18:50 2020 +0200\n> > >\n> > > libcamera: control_ids: Define draft controls\n> > >\n> > > > So ignore the discussion (if you want) :-P\n> > > >\n> > > > > Because there is no buffer assigned on the time-point where we\n> want to\n> > > > > capture the timestamp.\n> > > > >\n> > > > > The exact location of the timestamp is a \\todo\n> > > > >\n> > > > > * \\todo The sensor timestamp should be better\n> estimated\n> > > > > by connecting\n> > > > > * to the V4L2Device::frameStart signal.\n> > > > >\n> > > > > in all the pipeline-handlers as of now.\n> > > > >\n> > > > > We *want* to capture at frameStart signal, which emits in response\n> to\n> > > > > VIDIOC_DQEVENT, but we can't as no buffer assigned (yet)\n> > > > >\n> > > > > We probably need to assign a container taking care of timestamps\n> with\n> > > > > sequence numbers until a buffer is assigned down-the-line and then\n> set\n> > > > > the buffer metadata by reading seq# & timestamp from that\n> container.\n> > >\n> > > Should we just drop FrameBuffer::metadata_.timestamp in favour of\n> > > controls::SensorTimestamp ? What would be the drawbacks, if any ?\n> >\n> > Drawbacks for using Metadata control:\n> > - Have to parse the metadata control list to get timestamps\n> > (/sequences)\n>\n> The request metadata is also not available before the request fully\n> completes, while buffers will complete individually before. This is\n> something we may want to address in a generic way though, I suspect\n> there will be use cases for providing request metadata early.\n>\n> > Pro's for Metadata control:\n> > - Only one place for it, all buffers in the same request are defined as\n> > having the same value (sequence and timestamp).\n>\n> That's the part I like :-)\n>\n> > Drawbacks for per-buffer metadata:\n> > - Must be explictily assigned to keep them in sync even though they are\n> > the output of multiple video devices perhaps.\n> >\n> > Pro's for for per-buffer metadata:\n> > - Quicker to retrieve the values? (defined structure?)\n>\n> Possibly a bit quicker, but that seems marginal to me.\n>\n> > Any more on either side?\n> >\n> >\n> > Personally, I'm currently thinking these are better defined by the\n> > metadata() control list. After all, that's the purpose of the list\n> > right?\n>\n> I think so. I see very little value today in providing buffer completion\n> timestamps that would be different than the sensor timestamp (this could\n> possibly help measuring the processing time, but if that's the goal, it\n> could also be done by capturing the system timestamp in the buffer\n> completion handler on the application side, which would provide\n> statistics about the full processing time, including both the hardware\n> processing and the software processing).\n>\n\nThe above reason was exactly my rationale for keeping both :-)\nHowever, it's not something we rely on, or even use really, so\nI am happy to get rid of the buffer timestamps.\n\nNaush\n\n\n\n>\n> Let's wait for more feedback, but if there's a general agreement that\n> this is the direction we want to take, we could remove\n> FrameBuffer::metadata_.timestamp independently of addressing the sensor\n> sequence issue.\n>\n> > > > >>> double fps = ts - last_;\n> > > > >>> fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> > > > >>> last_ = ts;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 147E5BDB13\n\tfor ;\n\tThu, 9 Dec 2021 09:23:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4111C60882;\n\tThu, 9 Dec 2021 10:23:22 +0100 (CET)","from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F80460224\n\tfor ;\n\tThu, 9 Dec 2021 10:23:20 +0100 (CET)","by mail-lf1-x12b.google.com with SMTP id t26so10739676lfk.9\n\tfor ;\n\tThu, 09 Dec 2021 01:23:20 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ptxIYUbm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=UOvndgO42NsLZR0FNBqpMQuUG1ijoTeGiMDCN9FnTCM=;\n\tb=ptxIYUbm1JkwsYGoSerEuwZuyTb8XEWvht/6duuLHaa9F08F/+6eWeaIdtsUer5pOb\n\tVzOERNKyAr+dbPSpufH/xmTlpKV8dhCTx2dR8Xl/REDwehngW0yFnnUqHrw2EKsDEGo7\n\tcGcTHsTfZApPHY6WdVuvzAwUUPN+BquuArOF3zMuoxFnJaTKdR1FryNp8ib6D0wGrd/l\n\tWj1T5ZQUuM11LcNn99XPFcB8O7bMxn1Rsmy+P/vA+Y5TeY8mw3IV8wi8uyPJVd1+Tk2b\n\tjHYN6jpdOnOZvzvRXjAobm2wZmDqTplX06KZdTbNm6GDSasja0+jjsKJBC2ym9F8KiOF\n\t7quw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=UOvndgO42NsLZR0FNBqpMQuUG1ijoTeGiMDCN9FnTCM=;\n\tb=MX8PrljdtziGtDjEPAyFH+YeMKVUCLMEuNhHRfL5PEzrKdzdzVbAv0NdDgprbRMgIg\n\tR4sj7HRJLAHJnq9YKac1SW4s/yvsTWtirIsJSkzccCGVFeCYZh0cYxmuRu3F94BFr91X\n\t78Gf2LgOm3J+0phpcM+uJbBsTqN2f8ibD2Zxcmm2dGL5+ca+Vd+Sprz/kGIMkNZqFvsG\n\tAOTMQbtv4LniRvze1yjGwJlzW7OeAGbeWwAOUJml63mZqRaFhhALxLcWx8k6T7bwkf2p\n\tTDnobEu7a2g+cHupwMvlA/r3z4mAGf62uJeAVjpquktl5wRB0AD3Kd/DV5xL6ENjLKK+\n\tCQ+A==","X-Gm-Message-State":"AOAM532HSbEAAovCAzRqzS7+httb8lL3Cs7lLyAHajogpbETthWMMmD2\n\t6yqk3XJbLxqEiSj7p+sE48TFKVHKQiNnt/b+f42MJoz2hQrL+w==","X-Google-Smtp-Source":"ABdhPJyd6Sq0xV3y02yK+7hcqbCYzuQ0YYO0dIGYyccuAvPRJt7k2Y2yyV1kXlYH89JUaViGTSTPvwIW+nBi0JhvX2I=","X-Received":"by 2002:ac2:518f:: with SMTP id\n\tu15mr4842126lfi.161.1639041799701; \n\tThu, 09 Dec 2021 01:23:19 -0800 (PST)","MIME-Version":"1.0","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>\n\t\n\t<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>\n\t<5622f398-4a0e-3abd-24fe-ede344d478fe@ideasonboard.com>\n\t\n\t<163899716920.2066819.5739362313782502963@Monstersaurus>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 9 Dec 2021 09:23:03 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000003e5e8605d2b328b6\"","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23278,"web_url":"https://patchwork.libcamera.org/comment/23278/","msgid":"<8e94e1bf-6ecf-5112-b6fa-1fcde47a13ef@ideasonboard.com>","date":"2022-06-01T08:24:16","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch (again)\n\nOn 12/7/21 00:39, Kieran Bingham wrote:\n> The SensorTimestamp is defined to be the time of capture of the image.\n> While all streams should have the same timestamp, this is not always\n> defined or guaranteed as ISP drivers may not forward sequence numbers\n> and timestamps from their input buffer.\n>\n> Use the Request metadata to get the SensorTimestamp which must be\n> set by the pipeline handlers according to the data from the capture\n> device.\n>\n> Signed-off-by: Kieran Bingham \n\n\nMuch as has been discussed regarding the per-buffer timestamp vs request \ntimestamp. But it shouldn't affect this patch for now hence,\n\nReviewed-by: Umang Jain \n\n> ---\n> src/cam/camera_session.cpp | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index 1bf460fa3fb7..50170723c30f 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request *request)\n> \tconst Request::BufferMap &buffers = request->buffers();\n> \n> \t/*\n> -\t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> -\t * the first buffer, as all buffers should have matching timestamps.\n> +\t * Compute the frame rate. The timestamp is retrieved from the\n> +\t * SensorTimestamp property, though all streams should have the\n> +\t * same timestamp.\n> \t */\n> -\tuint64_t ts = buffers.begin()->second->metadata().timestamp;\n> +\tuint64_t ts = request->metadata().get(controls::SensorTimestamp);\n> \tdouble fps = ts - last_;\n> \tfps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> \tlast_ = ts;","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 2C1DBBD160\n\tfor ;\n\tWed, 1 Jun 2022 08:24:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 797FF633A7;\n\tWed, 1 Jun 2022 10:24:20 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 33AE460414\n\tfor ;\n\tWed, 1 Jun 2022 10:24:19 +0200 (CEST)","from [192.168.1.68] (235.red-81-36-186.dynamicip.rima-tde.net\n\t[81.36.186.235])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9ECF330A;\n\tWed, 1 Jun 2022 10:24:18 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654071860;\n\tbh=z8vxtwyiJ51YeKjlsv8KkH5XkqxAfz0+K8WfptWwvwM=;\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:\n\tFrom;\n\tb=OiVGhUxtBrWRvJ2O59liJRypLZd0hbSKDkQFs4FyeZUKT9loJUy0nezYz85lssoug\n\tDYqlTFw+5EZ6tBBG7P8YlVxvxSlQgqXhGJBGqkbSMxhfs4aL5/wM2zIF6/fXhfYNKz\n\toA07J5DNIL6JvPa/IVjAYnIC75b1EYWwBh7Y7dE94OZTfnLA9imFoX1H6qy0jzkhes\n\tJFdj9LFWSCbz2wynesUJfTqnpenhc2B7gi+++RpVJPZSmBTpDS1PRk7DRdN1B7AXGO\n\tmBMwqemwAZgoo0Kzru8aIAU8aMfGVk01sJzrufcZzshi4OFxk6v+8DjptdecUSiheY\n\tCi41b7SvhZh3g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654071858;\n\tbh=z8vxtwyiJ51YeKjlsv8KkH5XkqxAfz0+K8WfptWwvwM=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=kiwpEqWTw0r0nze+ICxLpbpVkqK00Sz+GpdRRE4k1d2HAODh6AibASFVyW6KxdKkn\n\tQerWRlqHw4RCJJmaNtrESFnpaNEl81JxqRSGArN8hnr6uQ8PRud4bedI4BdxQCh8c0\n\tUiHbbibGmudfgbEz6/f0lSAKksIZbDepe3IbQ7nU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"kiwpEqWT\"; dkim-atps=neutral","Message-ID":"<8e94e1bf-6ecf-5112-b6fa-1fcde47a13ef@ideasonboard.com>","Date":"Wed, 1 Jun 2022 10:24:16 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>","In-Reply-To":"<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23279,"web_url":"https://patchwork.libcamera.org/comment/23279/","msgid":"<8aec6e9b-01e6-9eb9-8fce-78b1b0239335@ideasonboard.com>","date":"2022-06-01T08:34:52","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi All,\n\nOn 12/8/21 21:59, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2021-12-08 18:57:10)\n>> Hi Umang,\n>>\n>> On Tue, Dec 07, 2021 at 07:07:46PM +0530, Umang Jain wrote:\n>>> On 12/7/21 6:51 PM, Umang Jain wrote:\n>>>> On 12/7/21 5:39 AM, Laurent Pinchart wrote:\n>>>>> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:\n>>>>>> The SensorTimestamp is defined to be the time of capture of the image.\n>>>>>> While all streams should have the same timestamp, this is not always\n>>>>>> defined or guaranteed as ISP drivers may not forward sequence numbers\n>>>>>> and timestamps from their input buffer.\n>>>>> That should then bo solved by the pipeline handler, which should store\n>>>>> the correct timestamp in the buffer metadata.\n>>>>>\n>>>>>> Use the Request metadata to get the SensorTimestamp which must be\n>>>>>> set by the pipeline handlers according to the data from the capture\n>>>>>> device.\n>>>>>>\n>>>>>> Signed-off-by: Kieran Bingham \n>>>>>> ---\n>>>>>>   src/cam/camera_session.cpp | 7 ++++---\n>>>>>>   1 file changed, 4 insertions(+), 3 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n>>>>>> index 1bf460fa3fb7..50170723c30f 100644\n>>>>>> --- a/src/cam/camera_session.cpp\n>>>>>> +++ b/src/cam/camera_session.cpp\n>>>>>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request\n>>>>>> *request)\n>>>>>>       const Request::BufferMap &buffers = request->buffers();\n>>>>>>         /*\n>>>>>> -     * Compute the frame rate. The timestamp is arbitrarily retrieved from\n>>>>>> -     * the first buffer, as all buffers should have matching timestamps.\n>>>>>> +     * Compute the frame rate. The timestamp is retrieved from the\n>>>>>> +     * SensorTimestamp property, though all streams should have the\n>>>>>> +     * same timestamp.\n>>>>>>        */\n>>>>>> -    uint64_t ts = buffers.begin()->second->metadata().timestamp;\n>>>>>> +    uint64_t ts = request->metadata().get(controls::SensorTimestamp);\n>>>>> This seems reasonable. Why do we have timestamps in the buffer metadata\n>>>>> ? :-)\n>>> Strong chance I have mis-understood the question, I later realized this\n>>> is a cam-related patch.\n>>>\n>>> to me, the question translated :\n>>>\n>>>     why do we have timestamps in the FrameBuffer.metadata_.timestamp ?\n>> To clarify, I meant to ask why we have both controls::SensorTimestamp\n>> *and* timestamps in individual buffers\n>> (FrameBuffer::metadata_.timestamp). I suppose it's historical, the\n>> timestamp member comes from\n>>\n>> commit e94e52c0cb27f92c085da6e776af8b3d3172bbb2\n>> Author: Niklas Söderlund \n>> Date: Thu Jan 24 23:34:51 2019 +0100\n>>\n>> libcamera: v4l2_device: Update dequeued buffer information\n>>\n>> while controls::SensorTimestamp has been introduced in\n>>\n>> commit 84e79bd8b5347d91309cbd759bbd988b5144fb8d\n>> Author: Jacopo Mondi \n>> Date: Thu Jul 30 15:18:50 2020 +0200\n>>\n>> libcamera: control_ids: Define draft controls\n>>\n>>> So ignore the discussion (if you want) :-P\n>>>\n>>>> Because there is no buffer assigned on the time-point where we want to\n>>>> capture the timestamp.\n>>>>\n>>>> The exact location of the timestamp is a \\todo\n>>>>\n>>>>              * \\todo The sensor timestamp should be better estimated\n>>>> by connecting\n>>>>              * to the V4L2Device::frameStart signal.\n>>>>\n>>>> in all the pipeline-handlers as of now.\n>>>>\n>>>> We *want* to capture at frameStart signal, which emits in response to\n>>>> VIDIOC_DQEVENT, but we can't as no buffer assigned (yet)\n>>>>\n>>>> We probably need to assign a container taking care of timestamps with\n>>>> sequence numbers until a buffer is assigned down-the-line and then set\n>>>> the buffer metadata by reading seq# & timestamp from that container.\n>> Should we just drop FrameBuffer::metadata_.timestamp in favour of\n>> controls::SensorTimestamp ? What would be the drawbacks, if any ?\n\n\nUnderstanding the 'why' per-framebuffer timestamp made me realize that\nit's a part of partial request completion implementation. From \napplication-developer\nguide:\n\n\"\nReceiving notifications about the single buffer\ncompletion event allows applications to implement partial request completion\nsupport, and to inspect the buffer content before the request it is part \nof has\nfully completed.\n\"\n\nSo that's why we have the timestamps in buffers. But it is also \nsomething that\ncan be done on applications' side as well (as discussed further in the \nseries)\n\n> Drawbacks for using Metadata control:\n> - Have to parse the metadata control list to get timestamps\n> (/sequences)\n>\n> Pro's for Metadata control:\n> - Only one place for it, all buffers in the same request are defined as\n> having the same value (sequence and timestamp).\n>\n>\n> Drawbacks for per-buffer metadata:\n> - Must be explictily assigned to keep them in sync even though they are\n> the output of multiple video devices perhaps.\n>\n> Pro's for for per-buffer metadata:\n> - Quicker to retrieve the values? (defined structure?)\n>\n> Any more on either side?\n>\n>\n> Personally, I'm currently thinking these are better defined by the\n> metadata() control list. After all, that's the purpose of the list\n> right?\n\n\nI might be pulling away the discussion to partial request completion side,\nbut a single metadata() control list can be returned at various points to\nhave a 'clear' partial completion implementation because currently?\nMight need to work with ControlList::merge() or ControlList::update() stuff,\nso it's a very separate topic indeed.\n\nSo I do think that metadata() control list is better for defining as you \nsuggested.\n\n>\n>\n>>>>>>       double fps = ts - last_;\n>>>>>>       fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n>>>>>>       last_ = ts;\n>> -- \n>> Regards,\n>>\n>> Laurent Pinchart","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 4E053BD161\n\tfor ;\n\tWed, 1 Jun 2022 08:34:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C0BD65635;\n\tWed, 1 Jun 2022 10:34:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F061B60414\n\tfor ;\n\tWed, 1 Jun 2022 10:34:55 +0200 (CEST)","from [192.168.1.68] (235.red-81-36-186.dynamicip.rima-tde.net\n\t[81.36.186.235])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 428B86F0;\n\tWed, 1 Jun 2022 10:34:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654072496;\n\tbh=eKoBBE4ASOjyWpN0L14XGnlnImJ9YFm80UMkTbQQRvE=;\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=W2nXPd4UDsA4SZb0L7BMNI6Tg5r7KsjFhUho/Vpc1S88GPZ8zz9Y00u8SK1yfkGh3\n\tP3Uh7aF9gbGnipJgW+HZkfJk4Ss5HHQ9eqbpktJO0VoO0KQjB70CdCbNU+gUkQ5Lfw\n\t602yLAdNCtkI66n1ZTL+bsCxBuEJ14vAQV4aodUY2q62DaAOCLVmWWOyI4/eB+6Vn5\n\tL6WhQDBLoSN3nIJ+5QFWp/EMQxge2k8j9bMtIs+y5ekteIdOPwR6A+1+CFGocKyGKt\n\t6sWFxSlWwQs9H3sQBrFNL0z2ppdyAzyeATbaUREsK5wf7AdMDEA0fTWSKfeGMiJ3b8\n\tfZELgkVlGKsSA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654072495;\n\tbh=eKoBBE4ASOjyWpN0L14XGnlnImJ9YFm80UMkTbQQRvE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=uCVxqCxHMUWJrD40i4eVGJb/wOJ0BOelMZHyRh6eCKvX8IAsavfvrzjnoW506vf5J\n\tGlpMz352CIl4PDTKZNmOsBB76hKSRZvH6HV3ICCw36o5q2nGzGjcHC0RZ00oYfPj9n\n\tPkSmwWa2YEz3qo7qL1UNdeI8ROqJaYQOuv3ewYFs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"uCVxqCxH\"; dkim-atps=neutral","Message-ID":"<8aec6e9b-01e6-9eb9-8fce-78b1b0239335@ideasonboard.com>","Date":"Wed, 1 Jun 2022 10:34:52 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tLaurent Pinchart ","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>\n\t\n\t<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>\n\t<5622f398-4a0e-3abd-24fe-ede344d478fe@ideasonboard.com>\n\t\n\t<163899716920.2066819.5739362313782502963@Monstersaurus>","In-Reply-To":"<163899716920.2066819.5739362313782502963@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23363,"web_url":"https://patchwork.libcamera.org/comment/23363/","msgid":"","date":"2022-06-08T22:40:14","subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Jun 01, 2022 at 10:34:52AM +0200, Umang Jain wrote:\n> On 12/8/21 21:59, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2021-12-08 18:57:10)\n> >> On Tue, Dec 07, 2021 at 07:07:46PM +0530, Umang Jain wrote:\n> >>> On 12/7/21 6:51 PM, Umang Jain wrote:\n> >>>> On 12/7/21 5:39 AM, Laurent Pinchart wrote:\n> >>>>> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:\n> >>>>>> The SensorTimestamp is defined to be the time of capture of the image.\n> >>>>>> While all streams should have the same timestamp, this is not always\n> >>>>>> defined or guaranteed as ISP drivers may not forward sequence numbers\n> >>>>>> and timestamps from their input buffer.\n> >>>>> That should then bo solved by the pipeline handler, which should store\n> >>>>> the correct timestamp in the buffer metadata.\n> >>>>>\n> >>>>>> Use the Request metadata to get the SensorTimestamp which must be\n> >>>>>> set by the pipeline handlers according to the data from the capture\n> >>>>>> device.\n> >>>>>>\n> >>>>>> Signed-off-by: Kieran Bingham \n> >>>>>> ---\n> >>>>>>   src/cam/camera_session.cpp | 7 ++++---\n> >>>>>>   1 file changed, 4 insertions(+), 3 deletions(-)\n> >>>>>>\n> >>>>>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> >>>>>> index 1bf460fa3fb7..50170723c30f 100644\n> >>>>>> --- a/src/cam/camera_session.cpp\n> >>>>>> +++ b/src/cam/camera_session.cpp\n> >>>>>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request\n> >>>>>> *request)\n> >>>>>>       const Request::BufferMap &buffers = request->buffers();\n> >>>>>>         /*\n> >>>>>> -     * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> >>>>>> -     * the first buffer, as all buffers should have matching timestamps.\n> >>>>>> +     * Compute the frame rate. The timestamp is retrieved from the\n> >>>>>> +     * SensorTimestamp property, though all streams should have the\n> >>>>>> +     * same timestamp.\n> >>>>>>        */\n> >>>>>> -    uint64_t ts = buffers.begin()->second->metadata().timestamp;\n> >>>>>> +    uint64_t ts = request->metadata().get(controls::SensorTimestamp);\n> >>>>> This seems reasonable. Why do we have timestamps in the buffer metadata\n> >>>>> ? :-)\n> >>> \n> >>> Strong chance I have mis-understood the question, I later realized this\n> >>> is a cam-related patch.\n> >>>\n> >>> to me, the question translated :\n> >>>\n> >>>     why do we have timestamps in the FrameBuffer.metadata_.timestamp ?\n> >> \n> >> To clarify, I meant to ask why we have both controls::SensorTimestamp\n> >> *and* timestamps in individual buffers\n> >> (FrameBuffer::metadata_.timestamp). I suppose it's historical, the\n> >> timestamp member comes from\n> >>\n> >> commit e94e52c0cb27f92c085da6e776af8b3d3172bbb2\n> >> Author: Niklas Söderlund \n> >> Date: Thu Jan 24 23:34:51 2019 +0100\n> >>\n> >> libcamera: v4l2_device: Update dequeued buffer information\n> >>\n> >> while controls::SensorTimestamp has been introduced in\n> >>\n> >> commit 84e79bd8b5347d91309cbd759bbd988b5144fb8d\n> >> Author: Jacopo Mondi \n> >> Date: Thu Jul 30 15:18:50 2020 +0200\n> >>\n> >> libcamera: control_ids: Define draft controls\n> >>\n> >>> So ignore the discussion (if you want) :-P\n> >>>\n> >>>> Because there is no buffer assigned on the time-point where we want to\n> >>>> capture the timestamp.\n> >>>>\n> >>>> The exact location of the timestamp is a \\todo\n> >>>>\n> >>>>              * \\todo The sensor timestamp should be better estimated\n> >>>> by connecting\n> >>>>              * to the V4L2Device::frameStart signal.\n> >>>>\n> >>>> in all the pipeline-handlers as of now.\n> >>>>\n> >>>> We *want* to capture at frameStart signal, which emits in response to\n> >>>> VIDIOC_DQEVENT, but we can't as no buffer assigned (yet)\n> >>>>\n> >>>> We probably need to assign a container taking care of timestamps with\n> >>>> sequence numbers until a buffer is assigned down-the-line and then set\n> >>>> the buffer metadata by reading seq# & timestamp from that container.\n> >> \n> >> Should we just drop FrameBuffer::metadata_.timestamp in favour of\n> >> controls::SensorTimestamp ? What would be the drawbacks, if any ?\n> \n> Understanding the 'why' per-framebuffer timestamp made me realize that\n> it's a part of partial request completion implementation. From \n> application-developer\n> guide:\n> \n> \"\n> Receiving notifications about the single buffer\n> completion event allows applications to implement partial request completion\n> support, and to inspect the buffer content before the request it is part of has\n> fully completed.\n> \"\n> \n> So that's why we have the timestamps in buffers. But it is also something that\n> can be done on applications' side as well (as discussed further in the \n> series)\n\nIf we need to provide timestamp information early, I'm tempted to do so\nthrough a separate start of frame event, similar to how Android does it.\nIt will provide the information, but also allow applications to react as\nearly as possible to the start of a frame.\n\n> > Drawbacks for using Metadata control:\n> > - Have to parse the metadata control list to get timestamps\n> > (/sequences)\n> >\n> > Pro's for Metadata control:\n> > - Only one place for it, all buffers in the same request are defined as\n> > having the same value (sequence and timestamp).\n> >\n> >\n> > Drawbacks for per-buffer metadata:\n> > - Must be explictily assigned to keep them in sync even though they are\n> > the output of multiple video devices perhaps.\n> >\n> > Pro's for for per-buffer metadata:\n> > - Quicker to retrieve the values? (defined structure?)\n> >\n> > Any more on either side?\n> >\n> >\n> > Personally, I'm currently thinking these are better defined by the\n> > metadata() control list. After all, that's the purpose of the list\n> > right?\n> \n> I might be pulling away the discussion to partial request completion side,\n> but a single metadata() control list can be returned at various points to\n> have a 'clear' partial completion implementation because currently?\n> Might need to work with ControlList::merge() or ControlList::update() stuff,\n> so it's a very separate topic indeed.\n\nWe don't support that yet, but if there's a use case, it could be\nimplemented, yes.\n\n> So I do think that metadata() control list is better for defining as you \n> suggested.\n> \n> >>>>>>       double fps = ts - last_;\n> >>>>>>       fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;\n> >>>>>>       last_ = ts;","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 A6AFCBD161\n\tfor ;\n\tWed, 8 Jun 2022 22:40:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D751A65635;\n\tThu, 9 Jun 2022 00:40:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3D641633A5\n\tfor ;\n\tThu, 9 Jun 2022 00:40:21 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 989CF6CF;\n\tThu, 9 Jun 2022 00:40:20 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654728022;\n\tbh=vp+Gf5uX38k3dGXOMVVXlgceoglV1TGxW+Kbz4jnTHM=;\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=yJiHYz1EcFFCNBHd8OuZnoU3n9/j4+g31U9ixpUZbVtNxaJwv4Jy0KT+vCQ1Jh1yW\n\tsDfVtP0/7aVo6UpxI3qMN+iSiMkq33nDrzEgcOCPj5XkZKxLD22ls+TSCe+hF331Le\n\tfpA4YD8ZkZ5bw1rIjvyvfm7RWqXqMgCGdjoh2WnVbYwqUNh/ChKEPsxfn7iy2k0/vq\n\thGUM4Qyb0zBUtCh2m8CvG8SzGctvM1utUfdEMnXavGPNHv6AKbWEOAMWBMJCdzuw0O\n\tTE0bv6aVp+UjcBu6+dmYeHwlIwKHE7NmP7o2neQf2C9OvoTdDejZQcaQ9UlsT2PGIi\n\t7dRzYL3Ka3GmQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654728020;\n\tbh=vp+Gf5uX38k3dGXOMVVXlgceoglV1TGxW+Kbz4jnTHM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c9GmvNj/SZIUnGkImMXBQAjcsR7xY6Eil7K8YXGLSBfa7PGpQLEBe/fFmzHLFgyz8\n\tJajOLB3N7L7ViO8rDdriZWk2YN0HTmsGQzGFWBumsK0Hz0IUnK5IDeZx+BQslTWkbl\n\tybk0KPk3KUQYr04PUG39SuhkfAKAPOh+bjTmGW6Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"c9GmvNj/\"; dkim-atps=neutral","Date":"Thu, 9 Jun 2022 01:40:14 +0300","To":"Umang Jain ","Message-ID":"","References":"<20211206233948.1351206-1-kieran.bingham@ideasonboard.com>\n\t<20211206233948.1351206-4-kieran.bingham@ideasonboard.com>\n\t\n\t<3ca0cecc-ba75-a663-aa9b-39bb3a2b2e28@ideasonboard.com>\n\t<5622f398-4a0e-3abd-24fe-ede344d478fe@ideasonboard.com>\n\t\n\t<163899716920.2066819.5739362313782502963@Monstersaurus>\n\t<8aec6e9b-01e6-9eb9-8fce-78b1b0239335@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<8aec6e9b-01e6-9eb9-8fce-78b1b0239335@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather\n\tthan buffer metadata","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]