[libcamera-devel] [PATCH 3/8] cam: Use SensorTimestamp rather than buffer metadata

[Umang Jain]

Hi,

On 12/9/21 2:43 AM, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Wed, Dec 08, 2021 at 08:59:29PM +0000, Kieran Bingham wrote:
>> Quoting Laurent Pinchart (2021-12-08 18:57:10)
>>> On Tue, Dec 07, 2021 at 07:07:46PM +0530, Umang Jain wrote:
>>>> On 12/7/21 6:51 PM, Umang Jain wrote:
>>>>> On 12/7/21 5:39 AM, Laurent Pinchart wrote:
>>>>>> On Mon, Dec 06, 2021 at 11:39:43PM +0000, Kieran Bingham wrote:
>>>>>>> The SensorTimestamp is defined to be the time of capture of the image.
>>>>>>> While all streams should have the same timestamp, this is not always
>>>>>>> defined or guaranteed as ISP drivers may not forward sequence numbers
>>>>>>> and timestamps from their input buffer.
>>>>>> That should then bo solved by the pipeline handler, which should store
>>>>>> the correct timestamp in the buffer metadata.
>>>>>>
>>>>>>> Use the Request metadata to get the SensorTimestamp which must be
>>>>>>> set by the pipeline handlers according to the data from the capture
>>>>>>> device.
>>>>>>>
>>>>>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>>>>> ---
>>>>>>>    src/cam/camera_session.cpp | 7 ++++---
>>>>>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
>>>>>>> index 1bf460fa3fb7..50170723c30f 100644
>>>>>>> --- a/src/cam/camera_session.cpp
>>>>>>> +++ b/src/cam/camera_session.cpp
>>>>>>> @@ -359,10 +359,11 @@ void CameraSession::processRequest(Request
>>>>>>> *request)
>>>>>>>        const Request::BufferMap &buffers = request->buffers();
>>>>>>>          /*
>>>>>>> -     * Compute the frame rate. The timestamp is arbitrarily retrieved from
>>>>>>> -     * the first buffer, as all buffers should have matching timestamps.
>>>>>>> +     * Compute the frame rate. The timestamp is retrieved from the
>>>>>>> +     * SensorTimestamp property, though all streams should have the
>>>>>>> +     * same timestamp.
>>>>>>>         */
>>>>>>> -    uint64_t ts = buffers.begin()->second->metadata().timestamp;
>>>>>>> +    uint64_t ts = request->metadata().get(controls::SensorTimestamp);
>>>>>> This seems reasonable. Why do we have timestamps in the buffer metadata
>>>>>> ? :-)
>>>> Strong chance I have mis-understood the question, I later realized this
>>>> is a cam-related patch.
>>>>
>>>> to me, the question translated :
>>>>
>>>>       why do we have timestamps in the FrameBuffer.metadata_.timestamp ?
>>> To clarify, I meant to ask why we have both controls::SensorTimestamp
>>> *and* timestamps in individual buffers
>>> (FrameBuffer::metadata_.timestamp). I suppose it's historical, the
>>> timestamp member comes from
>>>
>>> commit e94e52c0cb27f92c085da6e776af8b3d3172bbb2
>>> Author: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>> Date:   Thu Jan 24 23:34:51 2019 +0100
>>>
>>>      libcamera: v4l2_device: Update dequeued buffer information
>>>
>>> while controls::SensorTimestamp has been introduced in
>>>
>>> commit 84e79bd8b5347d91309cbd759bbd988b5144fb8d
>>> Author: Jacopo Mondi <jacopo at jmondi.org>
>>> Date:   Thu Jul 30 15:18:50 2020 +0200
>>>
>>>      libcamera: control_ids: Define draft controls
>>>
>>>> So ignore the discussion (if you want) :-P
>>>>
>>>>> Because there is no buffer assigned on the time-point where we want to
>>>>> capture the timestamp.
>>>>>
>>>>> The exact location of the timestamp is a \todo
>>>>>
>>>>>               * \todo The sensor timestamp should be better estimated
>>>>> by connecting
>>>>>               * to the V4L2Device::frameStart signal.
>>>>>
>>>>> in all the pipeline-handlers as of now.
>>>>>
>>>>> We *want* to capture at frameStart signal, which emits in response to
>>>>> VIDIOC_DQEVENT, but we can't as no buffer assigned (yet)
>>>>>
>>>>> We probably need to assign a container taking care of timestamps with
>>>>> sequence numbers until a buffer is assigned down-the-line and then set
>>>>> the buffer metadata by reading seq# & timestamp from that container.
>>> Should we just drop FrameBuffer::metadata_.timestamp in favour of
>>> controls::SensorTimestamp ? What would be the drawbacks, if any ?
>> Drawbacks for using Metadata control:
>>   - Have to parse the metadata control list to get timestamps
>>     (/sequences)
> The request metadata is also not available before the request fully
> completes, while buffers will complete individually before. This is
> something we may want to address in a generic way though, I suspect
> there will be use cases for providing request metadata early.
>
>> Pro's for Metadata control:
>>   - Only one place for it, all buffers in the same request are defined as
>>     having the same value (sequence and timestamp).
> That's the part I like :-)
>
>> Drawbacks for per-buffer metadata:
>>   - Must be explictily assigned to keep them in sync even though they are
>>     the output of multiple video devices perhaps.
>>
>> Pro's for for per-buffer metadata:
>>   - Quicker to retrieve the values? (defined structure?)
> Possibly a bit quicker, but that seems marginal to me.
>
>> Any more on either side?
>>
>>
>> Personally, I'm currently thinking these are better defined by the
>> metadata() control list. After all, that's the purpose of the list
>> right?
> I think so. I see very little value today in providing buffer completion
> timestamps that would be different than the sensor timestamp (this could


Agreed on this point. Applications 'generally' should be asking for 
request-completion timestamp and sensor-timestamp (if they want to use 
it for hw/processing metrics).

Buffer-completion timestamps are in-between (also sub-classed facts like 
"direct" buffers / post-processed buffers etc.)  - I can't see any major 
value for applications for requesting those, but I could be wrong!

> possibly help measuring the processing time, but if that's the goal, it
> could also be done by capturing the system timestamp in the buffer
> completion handler on the application side, which would provide
> statistics about the full processing time, including both the hardware
> processing and the software processing).
>
> Let's wait for more feedback, but if there's a general agreement that
> this is the direction we want to take, we could remove
> FrameBuffer::metadata_.timestamp independently of addressing the sensor
> sequence issue.


Makes sense.

>
>>>>>>>        double fps = ts - last_;
>>>>>>>        fps = last_ != 0 && fps ? 1000000000.0 / fps : 0.0;
>>>>>>>        last_ = ts;