[libcamera-devel] [PATCH 3/3] android: camera_device: Use controls::SensorTimestamp

[Laurent Pinchart]

Hi Jacopo,

On Fri, Apr 16, 2021 at 11:40:40AM +0200, Jacopo Mondi wrote:
> On Tue, Apr 13, 2021 at 09:43:31AM +0200, Jacopo Mondi wrote:
> > On Tue, Apr 13, 2021 at 05:50:01AM +0300, Laurent Pinchart wrote:
> > > On Wed, Apr 07, 2021 at 06:06:44PM +0200, Jacopo Mondi wrote:
> > > > Use the controls::SensorTimestamp value to populate
> > > > ANDROID_SENSOR_TIMESTAMP result metadata, if the Camera provides
> > > > it.
> > > >
> > > > Use the same control to notify the shutter even to the camera framework
> > >
> > > s/even/event/
> > >
> > > > otherwise fall-back to the timestamp of the first completed buffer
> > >
> > > s/fall-back/falls back/
> > >
> > > > if it is not available.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  src/android/camera_device.cpp | 33 ++++++++++++++++++++-------------
> > > >  src/android/camera_device.h   |  3 +--
> > > >  2 files changed, 21 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index 89044efa7ebe..749fe5c3dedc 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -2019,7 +2019,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > >
> > > >  void CameraDevice::requestComplete(Request *request)
> > > >  {
> > > > -	const Request::BufferMap &buffers = request->buffers();
> > > >  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
> > > >  	std::unique_ptr<CameraMetadata> resultMetadata;
> > > >  	Camera3RequestDescriptor *descriptor =
> > > > @@ -2034,14 +2033,7 @@ void CameraDevice::requestComplete(Request *request)
> > > >  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> > > >  			<< descriptor->buffers_.size() << " streams";
> > > >
> > > > -	/*
> > > > -	 * \todo The timestamp used for the metadata is currently always taken
> > > > -	 * from the first buffer (which may be the first stream) in the Request.
> > > > -	 * It might be appropriate to return a 'correct' (as determined by
> > > > -	 * pipeline handlers) timestamp in the Request itself.
> > > > -	 */
> > > > -	uint64_t timestamp = buffers.begin()->second->metadata().timestamp;
> > > > -	resultMetadata = getResultMetadata(*descriptor, timestamp);
> > > > +	resultMetadata = getResultMetadata(*descriptor);
> > > >
> > > >  	/* Handle any JPEG compression. */
> > > >  	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> > > > @@ -2086,6 +2078,19 @@ void CameraDevice::requestComplete(Request *request)
> > > >  	captureResult.output_buffers = descriptor->buffers_.data();
> > > >
> > > >  	if (status == CAMERA3_BUFFER_STATUS_OK) {
> > > > +		int64_t timestamp;
> > > > +
> > > > +		if (request->metadata().contains(controls::SensorTimestamp)) {
> > > > +			timestamp = request->metadata().get(controls::SensorTimestamp);
> > > > +		} else {
> > > > +			/*
> > > > +			 * Use the timestamp from the first buffer (which may be
> > > > +			 * the first stream) in the Request if the pipeline does
> > > > +			 * not report the sensor timestamp.
> > > > +			 */
> > > > +			const Request::BufferMap &buffers = request->buffers();
> > > > +			timestamp = buffers.begin()->second->metadata().timestamp;
> > > > +		}
> > > >  		notifyShutter(descriptor->frameNumber_, timestamp);
> > > >
> > > >  		captureResult.partial_result = 1;
> > > > @@ -2147,8 +2152,7 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
> > > >   * Produce a set of fixed result metadata.
> > > >   */
> > > >  std::unique_ptr<CameraMetadata>
> > > > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,
> > > > -				int64_t timestamp) const
> > > > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const
> > > >  {
> > > >  	const ControlList &metadata = descriptor.request_->metadata();
> > > >  	const CameraMetadata &settings = descriptor.settings_;
> > > > @@ -2274,8 +2278,6 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,
> > > >  	resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,
> > > >  				 &value32, 1);
> > > >
> > > > -	resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, &timestamp, 1);
> > > > -
> > > >  	value = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;
> > > >  	resultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,
> > > >  				 &value, 1);
> > > > @@ -2314,6 +2316,11 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,
> > > >  					 &exposure, 1);
> > > >  	}
> > > >
> > > > +	if (metadata.contains(controls::SensorTimestamp)) {
> > > > +		int64_t timestamp = metadata.get(controls::SensorTimestamp);
> > > > +		resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, &timestamp, 1);
> > > > +	}
> > >
> > > This will break other pipeline handlers. When it comes to the Android
> >
> > Why do you think they break ? They might not pass CTS but they're not
> > broken. Have I missed something ?
> >
> > > HAL, we care about at least UVC, rkisp1 and simple. Could you please
> > > include patches for those ? If it's not too much extra work, addressing
> >
> > To report the sensor timestamp ? I could, but I know it will need to
> > be adjusted to pass CTS.
> 
> All-in-all, I think each pipeline handler would need to find a way
> to correctly report the sensor timestamp, I don't think there's a
> catch-all solution. In particular:
> 
> - IPU3 we currently use the CIO2 timestamp, although the CIO2 driver
>   supports the FRAME_SYNC v4l2 event (and use it to update delayed
>   controls)
> - UVC afaict I can only rely on the completed buffer timestamp
> - RkISP1 I see it registering the V4L2_EVENT_FRAME_SYNC and the
>   pipeline uses that for delayed controls
> - RPi does the same
> - simple I cannot really tell as it depends on the platform in use and
>   currently there's no support for frame sync
> 
> I'm a bit hesitant to try to add support for all the platforms without
> being able to validate if the solution satisfies CTS, and I would
> rather plumb the control in the pipeline once the need to do so arises
> and we can test the implementation.
> 
> In the meantime I cannot really tell what the implications of
> non-reporting the sensor timestamp are. I assume it's not blocking for
> using the HAL, but surely compliance tests won't be pleased. Should we
> leave a default in place ? maybe using the completed buffer timestamp
> as the current implementation does ?

You're right that we don't know :-) Not passing CTS doesn't mean nothing
will work, but the camera service may choke on it.

You're also right that each pipeline handler needs specific support to
compute precise timestamp. Without solving that issue now, I think it
would make sense to start with the capture timestamp, as that's readily
available and won't require much work. We can then improve the
implementation in pipeline handlers individually at a later point. This
way we'll minimize the risk of the camera service or applications not
being able to use the camera at all.

> > > RPi and vimc would allow removing the fallback code in this patch.
> >
> > In general, for relevant features, like the shutter notification, I
> > would keep a fallback when it's sane to do so to ease integration of
> > new pipeline handlers.
> >
> > > > +
> > > >  	if (metadata.contains(controls::ScalerCrop)) {
> > > >  		Rectangle crop = metadata.get(controls::ScalerCrop);
> > > >  		int32_t cropRect[] = {
> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > index 11bdfec8d587..73e5009ac274 100644
> > > > --- a/src/android/camera_device.h
> > > > +++ b/src/android/camera_device.h
> > > > @@ -102,8 +102,7 @@ private:
> > > >  	libcamera::PixelFormat toPixelFormat(int format) const;
> > > >  	int processControls(Camera3RequestDescriptor *descriptor);
> > > >  	std::unique_ptr<CameraMetadata> getResultMetadata(
> > > > -		const Camera3RequestDescriptor &descriptor,
> > > > -		int64_t timestamp) const;
> > > > +		const Camera3RequestDescriptor &descriptor) const;
> > > >
> > > >  	unsigned int id_;
> > > >  	camera3_device_t camera3Device_;

-- 
Regards,

Laurent Pinchart