[libcamera-devel] [PATCH v2 2/2] ipa: ipu3: Provide frame timestamps through IPU3Event

[Umang Jain]

Hi Jacopo

On 5/27/21 1:31 AM, Jacopo Mondi wrote:
> Hi
>
> On Wed, May 26, 2021 at 04:54:32PM +0300, Laurent Pinchart wrote:
>> Hi Umang,
>>
>> Thank you for the patch.
>>
>> On Wed, May 26, 2021 at 06:40:25PM +0530, Umang Jain wrote:
>>> Pass in frame timestamps from IPU3 pipeline handler to IPU3 IPA via
>>> IPU3Event. Frame timestamps are helpful to IPA algorithms to
>>> convergence, by setting them via IPA stats.
>> This looks good. There's room for improvement, but on top, as it would
>> be too much yak-shaving:
>>
>> - Using std::chrono types would be nice for timestamps, but that should
>>    be done throughout libcamera.
> Just to mention that Nuash's utils::Duration will be a nice fit to
> replace all the custom timestampings in the library
>
> For the patch:
> - We create frameTimestamp by copying SensorTimestamp. Would it be
>    better to use the same name ?
>
The IPA is expecting a frame timestamp on it's side. It's entirely upto 
the PH to pass in, whatever it thinks closest as the timestamp of the 
frame. In this case, that's SensorTimestamp. I don't support that the 
naming should be changed just because we are passing in Sensor's 
timestamp. I am open to suggestions nevertheless, maybe an explicit 
comment on PH side :

     /* As of now, SensorTimestamp is the best approximation for the 
frame-timestamp */

Interestingly raspberry-pi IPA also does similar,

src/ipa/raspberrypi/raspberrypi.cpp:911:        int64_t frameTimestamp = 
data.controls.get(controls::SensorTimestamp);

>> - We should move away from IPU3Event, now that the IPA interface is
>>    specific to each pipeline handler. Instead of a single processEvent()
>>    method in IPAIPU3Interface, we should have 3 methods, with ad-hoc
>>    arguments.
>>
>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>>> ---
>>>   include/libcamera/ipa/ipu3.mojom     | 1 +
>>>   src/ipa/ipu3/ipu3.cpp                | 4 +++-
>>>   src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--
>>>   3 files changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom
>>> index 32c046ad..29b4c805 100644
>>> --- a/include/libcamera/ipa/ipu3.mojom
>>> +++ b/include/libcamera/ipa/ipu3.mojom
>>> @@ -21,6 +21,7 @@ enum IPU3Operations {
>>>   struct IPU3Event {
>>>   	IPU3Operations op;
>>>   	uint32 frame;
>>> +	int64 frameTimestamp;
>>>   	uint32 bufferId;
>>>   	libcamera.ControlList controls;
>>>   };
>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>> index 769c24d3..581297be 100644
>>> --- a/src/ipa/ipu3/ipu3.cpp
>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>> @@ -53,6 +53,7 @@ private:
>>>   	void processControls(unsigned int frame, const ControlList &controls);
>>>   	void fillParams(unsigned int frame, ipu3_uapi_params *params);
>>>   	void parseStatistics(unsigned int frame,
>>> +			     int64_t frameTimestamp,
>>>   			     const ipu3_uapi_stats_3a *stats);
>>>
>>>   	void setControls(unsigned int frame);
>>> @@ -214,7 +215,7 @@ void IPAIPU3::processEvent(const IPU3Event &event)
>>>   		const ipu3_uapi_stats_3a *stats =
>>>   			reinterpret_cast<ipu3_uapi_stats_3a *>(mem.data());
>>>
>>> -		parseStatistics(event.frame, stats);
>>> +		parseStatistics(event.frame, event.frameTimestamp, stats);
>>>   		break;
>>>   	}
>>>   	case EventFillParams: {
>>> @@ -257,6 +258,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>>>   }
>>>
>>>   void IPAIPU3::parseStatistics(unsigned int frame,
>>> +			      [[maybe_unused]] int64_t frameTimestamp,
>>>   			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>>>   {
>>>   	ControlList ctrls(controls::controls);
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index 750880ed..58923bc7 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -1357,6 +1357,8 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>>   	if (!info)
>>>   		return;
>>>
>>> +	Request *request = info->request;
>>> +
>>>   	if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
>>>   		info->metadataProcessed = true;
>>>
>>> @@ -1364,8 +1366,6 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>>   		 * tryComplete() will delete info if it completes the IPU3Frame.
>>>   		 * In that event, we must have obtained the Request before hand.
>>>   		 */
>>> -		Request *request = info->request;
>>> -
>>>   		if (frameInfos_.tryComplete(info))
>>>   			pipe_->completeRequest(request);
>>>
>>> @@ -1376,6 +1376,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>>   	ev.op = ipa::ipu3::EventStatReady;
>>>   	ev.frame = info->id;
>>>   	ev.bufferId = info->statBuffer->cookie();
>>> +	ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp);
>>>   	ipa_->processEvent(ev);
>>>   }
>>>
>> --
>> Regards,
>>
>> Laurent Pinchart