| Message ID | 20210407160644.58326-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, thanks for the patch. On Thu, Apr 8, 2021 at 1:06 AM Jacopo Mondi <jacopo@jmondi.org> 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 > otherwise fall-back to the timestamp of the first completed buffer > if it is not available. > > Signed-off-by: Jacopo Mondi <jacopo@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. > + */ I would leave a TODO comment of removing this after every pipeline fills a timestamp to the metadata. > + 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, ×tamp, 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, ×tamp, 1); > + } > + > 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_; > -- Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. 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@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, ×tamp, 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, ×tamp, 1); > + } This will break other pipeline handlers. When it comes to the Android 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 RPi and vimc would allow removing the fallback code in this patch. > + > 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_;
Hi Laurent, On Tue, Apr 13, 2021 at 05:50:01AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > 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@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, ×tamp, 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, ×tamp, 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. > 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. Thanks j > > + > > 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
Hi again, a bit more thoughts on this topic... On Tue, Apr 13, 2021 at 09:43:31AM +0200, Jacopo Mondi wrote: > Hi Laurent, > > On Tue, Apr 13, 2021 at 05:50:01AM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > 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@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, ×tamp, 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, ×tamp, 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 ? Thanks j > > 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. > > Thanks > j > > > > + > > > 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 > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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, ×tamp, 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, ×tamp, 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_;
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, ×tamp, 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, ×tamp, 1); + } + 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_;
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 otherwise fall-back to the timestamp of the first completed buffer if it is not available. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/android/camera_device.cpp | 33 ++++++++++++++++++++------------- src/android/camera_device.h | 3 +-- 2 files changed, 21 insertions(+), 15 deletions(-)