[{"id":16152,"web_url":"https://patchwork.libcamera.org/comment/16152/","msgid":"","date":"2021-04-08T09:11:53","subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Use\n\tcontrols::SensorTimestamp","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thanks for the patch.\n\nOn Thu, Apr 8, 2021 at 1:06 AM Jacopo Mondi wrote:\n>\n> Use the controls::SensorTimestamp value to populate\n> ANDROID_SENSOR_TIMESTAMP result metadata, if the Camera provides\n> it.\n>\n> Use the same control to notify the shutter even to the camera framework\n> otherwise fall-back to the timestamp of the first completed buffer\n> if it is not available.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 33 ++++++++++++++++++++-------------\n> src/android/camera_device.h | 3 +--\n> 2 files changed, 21 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 89044efa7ebe..749fe5c3dedc 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -2019,7 +2019,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>\n> void CameraDevice::requestComplete(Request *request)\n> {\n> - const Request::BufferMap &buffers = request->buffers();\n> camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> std::unique_ptr resultMetadata;\n> Camera3RequestDescriptor *descriptor =\n> @@ -2034,14 +2033,7 @@ void CameraDevice::requestComplete(Request *request)\n> LOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> << descriptor->buffers_.size() << \" streams\";\n>\n> - /*\n> - * \\todo The timestamp used for the metadata is currently always taken\n> - * from the first buffer (which may be the first stream) in the Request.\n> - * It might be appropriate to return a 'correct' (as determined by\n> - * pipeline handlers) timestamp in the Request itself.\n> - */\n> - uint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> - resultMetadata = getResultMetadata(*descriptor, timestamp);\n> + resultMetadata = getResultMetadata(*descriptor);\n>\n> /* Handle any JPEG compression. */\n> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> @@ -2086,6 +2078,19 @@ void CameraDevice::requestComplete(Request *request)\n> captureResult.output_buffers = descriptor->buffers_.data();\n>\n> if (status == CAMERA3_BUFFER_STATUS_OK) {\n> + int64_t timestamp;\n> +\n> + if (request->metadata().contains(controls::SensorTimestamp)) {\n> + timestamp = request->metadata().get(controls::SensorTimestamp);\n> + } else {\n> + /*\n> + * Use the timestamp from the first buffer (which may be\n> + * the first stream) in the Request if the pipeline does\n> + * not report the sensor timestamp.\n> + */\n\nI would leave a TODO comment of removing this after every pipeline\nfills a timestamp to the metadata.\n\n> + const Request::BufferMap &buffers = request->buffers();\n> + timestamp = buffers.begin()->second->metadata().timestamp;\n> + }\n> notifyShutter(descriptor->frameNumber_, timestamp);\n>\n> captureResult.partial_result = 1;\n> @@ -2147,8 +2152,7 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> * Produce a set of fixed result metadata.\n> */\n> std::unique_ptr\n> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> - int64_t timestamp) const\n> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> {\n> const ControlList &metadata = descriptor.request_->metadata();\n> const CameraMetadata &settings = descriptor.settings_;\n> @@ -2274,8 +2278,6 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> resultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,\n> &value32, 1);\n>\n> - resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);\n> -\n> value = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> resultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,\n> &value, 1);\n> @@ -2314,6 +2316,11 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> &exposure, 1);\n> }\n>\n> + if (metadata.contains(controls::SensorTimestamp)) {\n> + int64_t timestamp = metadata.get(controls::SensorTimestamp);\n> + resultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);\n> + }\n> +\n> if (metadata.contains(controls::ScalerCrop)) {\n> Rectangle crop = metadata.get(controls::ScalerCrop);\n> int32_t cropRect[] = {\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 11bdfec8d587..73e5009ac274 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -102,8 +102,7 @@ private:\n> libcamera::PixelFormat toPixelFormat(int format) const;\n> int processControls(Camera3RequestDescriptor *descriptor);\n> std::unique_ptr getResultMetadata(\n> - const Camera3RequestDescriptor &descriptor,\n> - int64_t timestamp) const;\n> + const Camera3RequestDescriptor &descriptor) const;\n>\n> unsigned int id_;\n> camera3_device_t camera3Device_;\n> --\n\nReviewed-by: Hirokazu Honda \n> 2.31.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 C2AD0BD16B\n\tfor ;\n\tThu, 8 Apr 2021 09:12:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3BE6F687F5;\n\tThu, 8 Apr 2021 11:12:04 +0200 (CEST)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F552687F2\n\tfor ;\n\tThu, 8 Apr 2021 11:12:03 +0200 (CEST)","by mail-ed1-x52d.google.com with SMTP id 18so1514677edx.3\n\tfor ;\n\tThu, 08 Apr 2021 02:12:03 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"cA9HQipK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=IBIGvq3rzFjpNvMlAVSYBAcJMOIiG35h8Bcu3xM8m58=;\n\tb=cA9HQipKSZ4bsupAKr9Z9PLR0f3uSKknQE2y2bsbWnEP7YW6wFgJhscZnm50+69/+o\n\tJALLL07L/GB2ydsdpZpylHVV9obiKnUolP/iSbhWQfUOjYGowrKTwcCpIQO5LniRlObV\n\tMri+lje7uuNKI5H1GTdzhVU39J34i/GrzCUWs=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=IBIGvq3rzFjpNvMlAVSYBAcJMOIiG35h8Bcu3xM8m58=;\n\tb=H4zQCDT0dj9KhtHhX5WwnaE6zHl3X2Aj3h2LCMantQgh4hxs2qP2qcFWEfYwCmofuq\n\tUzGKL3df8hLCI6BZIahIqS1OoRCYpwxLxP5EYocgh1bgJG6H+oltEz/3kBO5JPh7TFAQ\n\t9gEBlhfeMGubPaj3P1rSW2hl+ogb8jkcjIyOarZIsr+jv21lFyQq289t8EP/UzvBgf16\n\tSrFWw0ydvjva5JHFmwr5COkHeOsTvZRWEkyKYbdgX6/dTMiPbfkCgc4zTHG2MGwQBsYx\n\t9I0p8XBEsU4B6SxHVp8IMrnQO+kekUJ4XEPDFZIAlYHBXtFIpt+70CvRuiCy5avVLJwI\n\theWQ==","X-Gm-Message-State":"AOAM530/odHkLgxfD8gRrxt0ILelilQo3SxHwmzV0PmhXvCZNM1uSDuS\n\t7K5YXAlXde3lNXu1QwcvAB4NKiwwniRxcpTj0C9pyQ==","X-Google-Smtp-Source":"ABdhPJyx5jIIZxNUSZGrbf0huTIJWa65pjiYVlZWY3LQP6iQydqd+gE+RTAdekJEHSg1qjNIYiIu47Ka6WhXU7pgwgo=","X-Received":"by 2002:a05:6402:5211:: with SMTP id\n\ts17mr9996697edd.327.1617873123112; \n\tThu, 08 Apr 2021 02:12:03 -0700 (PDT)","MIME-Version":"1.0","References":"<20210407160644.58326-1-jacopo@jmondi.org>\n\t<20210407160644.58326-4-jacopo@jmondi.org>","In-Reply-To":"<20210407160644.58326-4-jacopo@jmondi.org>","From":"Hirokazu Honda ","Date":"Thu, 8 Apr 2021 18:11:53 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Use\n\tcontrols::SensorTimestamp","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16208,"web_url":"https://patchwork.libcamera.org/comment/16208/","msgid":"","date":"2021-04-13T02:50:01","subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Use\n\tcontrols::SensorTimestamp","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Apr 07, 2021 at 06:06:44PM +0200, Jacopo Mondi wrote:\n> Use the controls::SensorTimestamp value to populate\n> ANDROID_SENSOR_TIMESTAMP result metadata, if the Camera provides\n> it.\n> \n> Use the same control to notify the shutter even to the camera framework\n\ns/even/event/\n\n> otherwise fall-back to the timestamp of the first completed buffer\n\ns/fall-back/falls back/\n\n> if it is not available.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 33 ++++++++++++++++++++-------------\n> src/android/camera_device.h | 3 +--\n> 2 files changed, 21 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 89044efa7ebe..749fe5c3dedc 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -2019,7 +2019,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> void CameraDevice::requestComplete(Request *request)\n> {\n> -\tconst Request::BufferMap &buffers = request->buffers();\n> \tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> \tstd::unique_ptr resultMetadata;\n> \tCamera3RequestDescriptor *descriptor =\n> @@ -2034,14 +2033,7 @@ void CameraDevice::requestComplete(Request *request)\n> \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> \t\t\t<< descriptor->buffers_.size() << \" streams\";\n> \n> -\t/*\n> -\t * \\todo The timestamp used for the metadata is currently always taken\n> -\t * from the first buffer (which may be the first stream) in the Request.\n> -\t * It might be appropriate to return a 'correct' (as determined by\n> -\t * pipeline handlers) timestamp in the Request itself.\n> -\t */\n> -\tuint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> -\tresultMetadata = getResultMetadata(*descriptor, timestamp);\n> +\tresultMetadata = getResultMetadata(*descriptor);\n> \n> \t/* Handle any JPEG compression. */\n> \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> @@ -2086,6 +2078,19 @@ void CameraDevice::requestComplete(Request *request)\n> \tcaptureResult.output_buffers = descriptor->buffers_.data();\n> \n> \tif (status == CAMERA3_BUFFER_STATUS_OK) {\n> +\t\tint64_t timestamp;\n> +\n> +\t\tif (request->metadata().contains(controls::SensorTimestamp)) {\n> +\t\t\ttimestamp = request->metadata().get(controls::SensorTimestamp);\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * Use the timestamp from the first buffer (which may be\n> +\t\t\t * the first stream) in the Request if the pipeline does\n> +\t\t\t * not report the sensor timestamp.\n> +\t\t\t */\n> +\t\t\tconst Request::BufferMap &buffers = request->buffers();\n> +\t\t\ttimestamp = buffers.begin()->second->metadata().timestamp;\n> +\t\t}\n> \t\tnotifyShutter(descriptor->frameNumber_, timestamp);\n> \n> \t\tcaptureResult.partial_result = 1;\n> @@ -2147,8 +2152,7 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> * Produce a set of fixed result metadata.\n> */\n> std::unique_ptr\n> -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> -\t\t\t\tint64_t timestamp) const\n> +CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> {\n> \tconst ControlList &metadata = descriptor.request_->metadata();\n> \tconst CameraMetadata &settings = descriptor.settings_;\n> @@ -2274,8 +2278,6 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> \tresultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,\n> \t\t\t\t &value32, 1);\n> \n> -\tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);\n> -\n> \tvalue = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> \tresultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,\n> \t\t\t\t &value, 1);\n> @@ -2314,6 +2316,11 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> \t\t\t\t\t &exposure, 1);\n> \t}\n> \n> +\tif (metadata.contains(controls::SensorTimestamp)) {\n> +\t\tint64_t timestamp = metadata.get(controls::SensorTimestamp);\n> +\t\tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);\n> +\t}\n\nThis will break other pipeline handlers. When it comes to the Android\nHAL, we care about at least UVC, rkisp1 and simple. Could you please\ninclude patches for those ? If it's not too much extra work, addressing\nRPi and vimc would allow removing the fallback code in this patch.\n\n> +\n> \tif (metadata.contains(controls::ScalerCrop)) {\n> \t\tRectangle crop = metadata.get(controls::ScalerCrop);\n> \t\tint32_t cropRect[] = {\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 11bdfec8d587..73e5009ac274 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -102,8 +102,7 @@ private:\n> \tlibcamera::PixelFormat toPixelFormat(int format) const;\n> \tint processControls(Camera3RequestDescriptor *descriptor);\n> \tstd::unique_ptr getResultMetadata(\n> -\t\tconst Camera3RequestDescriptor &descriptor,\n> -\t\tint64_t timestamp) const;\n> +\t\tconst Camera3RequestDescriptor &descriptor) const;\n> \n> \tunsigned int id_;\n> \tcamera3_device_t camera3Device_;","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 16764BD224\n\tfor ;\n\tTue, 13 Apr 2021 02:50:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 898C6687F6;\n\tTue, 13 Apr 2021 04:50:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CEA0605AE\n\tfor ;\n\tTue, 13 Apr 2021 04:50:51 +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 D21916F2;\n\tTue, 13 Apr 2021 04:50:50 +0200 (CEST)"],"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=\"tc5ECJg/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618282251;\n\tbh=BcQVBEEFXgaQOC1wDW1B9qx9rmDJcIqZg2i27vpdOhY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tc5ECJg/jL1czxwRRjbHIFm2Sqsqs9CQs6EmjDaBSNr0VrNKZ3dCwxZbc7V0IEY5c\n\twvGLr/JxLas4gji9YW6jWVLA0L8VotWgGI+rJWXbE7ywIjVdL+xFzj4Jn6myPjPpM/\n\t8nHvLXhHE1EEhcMQycAQ8mrNYZZ4qfnm6x9ZtrDc=","Date":"Tue, 13 Apr 2021 05:50:01 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210407160644.58326-1-jacopo@jmondi.org>\n\t<20210407160644.58326-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210407160644.58326-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Use\n\tcontrols::SensorTimestamp","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16220,"web_url":"https://patchwork.libcamera.org/comment/16220/","msgid":"<20210413074331.fitbqvyjh76tmmju@uno.localdomain>","date":"2021-04-13T07:43:31","subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Use\n\tcontrols::SensorTimestamp","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Apr 13, 2021 at 05:50:01AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Apr 07, 2021 at 06:06:44PM +0200, Jacopo Mondi wrote:\n> > Use the controls::SensorTimestamp value to populate\n> > ANDROID_SENSOR_TIMESTAMP result metadata, if the Camera provides\n> > it.\n> >\n> > Use the same control to notify the shutter even to the camera framework\n>\n> s/even/event/\n>\n> > otherwise fall-back to the timestamp of the first completed buffer\n>\n> s/fall-back/falls back/\n>\n> > if it is not available.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 33 ++++++++++++++++++++-------------\n> > src/android/camera_device.h | 3 +--\n> > 2 files changed, 21 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 89044efa7ebe..749fe5c3dedc 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -2019,7 +2019,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >\n> > void CameraDevice::requestComplete(Request *request)\n> > {\n> > -\tconst Request::BufferMap &buffers = request->buffers();\n> > \tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > \tstd::unique_ptr resultMetadata;\n> > \tCamera3RequestDescriptor *descriptor =\n> > @@ -2034,14 +2033,7 @@ void CameraDevice::requestComplete(Request *request)\n> > \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> > \t\t\t<< descriptor->buffers_.size() << \" streams\";\n> >\n> > -\t/*\n> > -\t * \\todo The timestamp used for the metadata is currently always taken\n> > -\t * from the first buffer (which may be the first stream) in the Request.\n> > -\t * It might be appropriate to return a 'correct' (as determined by\n> > -\t * pipeline handlers) timestamp in the Request itself.\n> > -\t */\n> > -\tuint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> > -\tresultMetadata = getResultMetadata(*descriptor, timestamp);\n> > +\tresultMetadata = getResultMetadata(*descriptor);\n> >\n> > \t/* Handle any JPEG compression. */\n> > \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > @@ -2086,6 +2078,19 @@ void CameraDevice::requestComplete(Request *request)\n> > \tcaptureResult.output_buffers = descriptor->buffers_.data();\n> >\n> > \tif (status == CAMERA3_BUFFER_STATUS_OK) {\n> > +\t\tint64_t timestamp;\n> > +\n> > +\t\tif (request->metadata().contains(controls::SensorTimestamp)) {\n> > +\t\t\ttimestamp = request->metadata().get(controls::SensorTimestamp);\n> > +\t\t} else {\n> > +\t\t\t/*\n> > +\t\t\t * Use the timestamp from the first buffer (which may be\n> > +\t\t\t * the first stream) in the Request if the pipeline does\n> > +\t\t\t * not report the sensor timestamp.\n> > +\t\t\t */\n> > +\t\t\tconst Request::BufferMap &buffers = request->buffers();\n> > +\t\t\ttimestamp = buffers.begin()->second->metadata().timestamp;\n> > +\t\t}\n> > \t\tnotifyShutter(descriptor->frameNumber_, timestamp);\n> >\n> > \t\tcaptureResult.partial_result = 1;\n> > @@ -2147,8 +2152,7 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> > * Produce a set of fixed result metadata.\n> > */\n> > std::unique_ptr\n> > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> > -\t\t\t\tint64_t timestamp) const\n> > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> > {\n> > \tconst ControlList &metadata = descriptor.request_->metadata();\n> > \tconst CameraMetadata &settings = descriptor.settings_;\n> > @@ -2274,8 +2278,6 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> > \tresultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,\n> > \t\t\t\t &value32, 1);\n> >\n> > -\tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);\n> > -\n> > \tvalue = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> > \tresultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,\n> > \t\t\t\t &value, 1);\n> > @@ -2314,6 +2316,11 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> > \t\t\t\t\t &exposure, 1);\n> > \t}\n> >\n> > +\tif (metadata.contains(controls::SensorTimestamp)) {\n> > +\t\tint64_t timestamp = metadata.get(controls::SensorTimestamp);\n> > +\t\tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);\n> > +\t}\n>\n> This will break other pipeline handlers. When it comes to the Android\n\nWhy do you think they break ? They might not pass CTS but they're not\nbroken. Have I missed something ?\n\n> HAL, we care about at least UVC, rkisp1 and simple. Could you please\n> include patches for those ? If it's not too much extra work, addressing\n\nTo report the sensor timestamp ? I could, but I know it will need to\nbe adjusted to pass CTS.\n\n> RPi and vimc would allow removing the fallback code in this patch.\n>\n\nIn general, for relevant features, like the shutter notification, I\nwould keep a fallback when it's sane to do so to ease integration of\nnew pipeline handlers.\n\nThanks\n j\n\n> > +\n> > \tif (metadata.contains(controls::ScalerCrop)) {\n> > \t\tRectangle crop = metadata.get(controls::ScalerCrop);\n> > \t\tint32_t cropRect[] = {\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 11bdfec8d587..73e5009ac274 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -102,8 +102,7 @@ private:\n> > \tlibcamera::PixelFormat toPixelFormat(int format) const;\n> > \tint processControls(Camera3RequestDescriptor *descriptor);\n> > \tstd::unique_ptr getResultMetadata(\n> > -\t\tconst Camera3RequestDescriptor &descriptor,\n> > -\t\tint64_t timestamp) const;\n> > +\t\tconst Camera3RequestDescriptor &descriptor) const;\n> >\n> > \tunsigned int id_;\n> > \tcamera3_device_t camera3Device_;\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 11947BD224\n\tfor ;\n\tTue, 13 Apr 2021 07:42:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFFFA68800;\n\tTue, 13 Apr 2021 09:42:53 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DBB72687F3\n\tfor ;\n\tTue, 13 Apr 2021 09:42:52 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 7119D240010;\n\tTue, 13 Apr 2021 07:42:52 +0000 (UTC)"],"Date":"Tue, 13 Apr 2021 09:43:31 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210413074331.fitbqvyjh76tmmju@uno.localdomain>","References":"<20210407160644.58326-1-jacopo@jmondi.org>\n\t<20210407160644.58326-4-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Use\n\tcontrols::SensorTimestamp","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16309,"web_url":"https://patchwork.libcamera.org/comment/16309/","msgid":"<20210416094040.sxpnetl743u7ed7b@uno.localdomain>","date":"2021-04-16T09:40:40","subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Use\n\tcontrols::SensorTimestamp","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi again,\n a bit more thoughts on this topic...\n\nOn Tue, Apr 13, 2021 at 09:43:31AM +0200, Jacopo Mondi wrote:\n> Hi Laurent,\n>\n> On Tue, Apr 13, 2021 at 05:50:01AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Apr 07, 2021 at 06:06:44PM +0200, Jacopo Mondi wrote:\n> > > Use the controls::SensorTimestamp value to populate\n> > > ANDROID_SENSOR_TIMESTAMP result metadata, if the Camera provides\n> > > it.\n> > >\n> > > Use the same control to notify the shutter even to the camera framework\n> >\n> > s/even/event/\n> >\n> > > otherwise fall-back to the timestamp of the first completed buffer\n> >\n> > s/fall-back/falls back/\n> >\n> > > if it is not available.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 33 ++++++++++++++++++++-------------\n> > > src/android/camera_device.h | 3 +--\n> > > 2 files changed, 21 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 89044efa7ebe..749fe5c3dedc 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -2019,7 +2019,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >\n> > > void CameraDevice::requestComplete(Request *request)\n> > > {\n> > > -\tconst Request::BufferMap &buffers = request->buffers();\n> > > \tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > > \tstd::unique_ptr resultMetadata;\n> > > \tCamera3RequestDescriptor *descriptor =\n> > > @@ -2034,14 +2033,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> > > \t\t\t<< descriptor->buffers_.size() << \" streams\";\n> > >\n> > > -\t/*\n> > > -\t * \\todo The timestamp used for the metadata is currently always taken\n> > > -\t * from the first buffer (which may be the first stream) in the Request.\n> > > -\t * It might be appropriate to return a 'correct' (as determined by\n> > > -\t * pipeline handlers) timestamp in the Request itself.\n> > > -\t */\n> > > -\tuint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> > > -\tresultMetadata = getResultMetadata(*descriptor, timestamp);\n> > > +\tresultMetadata = getResultMetadata(*descriptor);\n> > >\n> > > \t/* Handle any JPEG compression. */\n> > > \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > > @@ -2086,6 +2078,19 @@ void CameraDevice::requestComplete(Request *request)\n> > > \tcaptureResult.output_buffers = descriptor->buffers_.data();\n> > >\n> > > \tif (status == CAMERA3_BUFFER_STATUS_OK) {\n> > > +\t\tint64_t timestamp;\n> > > +\n> > > +\t\tif (request->metadata().contains(controls::SensorTimestamp)) {\n> > > +\t\t\ttimestamp = request->metadata().get(controls::SensorTimestamp);\n> > > +\t\t} else {\n> > > +\t\t\t/*\n> > > +\t\t\t * Use the timestamp from the first buffer (which may be\n> > > +\t\t\t * the first stream) in the Request if the pipeline does\n> > > +\t\t\t * not report the sensor timestamp.\n> > > +\t\t\t */\n> > > +\t\t\tconst Request::BufferMap &buffers = request->buffers();\n> > > +\t\t\ttimestamp = buffers.begin()->second->metadata().timestamp;\n> > > +\t\t}\n> > > \t\tnotifyShutter(descriptor->frameNumber_, timestamp);\n> > >\n> > > \t\tcaptureResult.partial_result = 1;\n> > > @@ -2147,8 +2152,7 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> > > * Produce a set of fixed result metadata.\n> > > */\n> > > std::unique_ptr\n> > > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> > > -\t\t\t\tint64_t timestamp) const\n> > > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> > > {\n> > > \tconst ControlList &metadata = descriptor.request_->metadata();\n> > > \tconst CameraMetadata &settings = descriptor.settings_;\n> > > @@ -2274,8 +2278,6 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> > > \tresultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,\n> > > \t\t\t\t &value32, 1);\n> > >\n> > > -\tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);\n> > > -\n> > > \tvalue = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> > > \tresultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,\n> > > \t\t\t\t &value, 1);\n> > > @@ -2314,6 +2316,11 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> > > \t\t\t\t\t &exposure, 1);\n> > > \t}\n> > >\n> > > +\tif (metadata.contains(controls::SensorTimestamp)) {\n> > > +\t\tint64_t timestamp = metadata.get(controls::SensorTimestamp);\n> > > +\t\tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);\n> > > +\t}\n> >\n> > This will break other pipeline handlers. When it comes to the Android\n>\n> Why do you think they break ? They might not pass CTS but they're not\n> broken. Have I missed something ?\n>\n> > HAL, we care about at least UVC, rkisp1 and simple. Could you please\n> > include patches for those ? If it's not too much extra work, addressing\n>\n> To report the sensor timestamp ? I could, but I know it will need to\n> be adjusted to pass CTS.\n>\n\nAll-in-all, I think each pipeline handler would need to find a way\nto correctly report the sensor timestamp, I don't think there's a\ncatch-all solution. In particular:\n\n- IPU3 we currently use the CIO2 timestamp, although the CIO2 driver\n supports the FRAME_SYNC v4l2 event (and use it to update delayed\n controls)\n- UVC afaict I can only rely on the completed buffer timestamp\n- RkISP1 I see it registering the V4L2_EVENT_FRAME_SYNC and the\n pipeline uses that for delayed controls\n- RPi does the same\n- simple I cannot really tell as it depends on the platform in use and\n currently there's no support for frame sync\n\nI'm a bit hesitant to try to add support for all the platforms without\nbeing able to validate if the solution satisfies CTS, and I would\nrather plumb the control in the pipeline once the need to do so arises\nand we can test the implementation.\n\nIn the meantime I cannot really tell what the implications of\nnon-reporting the sensor timestamp are. I assume it's not blocking for\nusing the HAL, but surely compliance tests won't be pleased. Should we\nleave a default in place ? maybe using the completed buffer timestamp\nas the current implementation does ?\n\nThanks\n j\n\n> > RPi and vimc would allow removing the fallback code in this patch.\n> >\n>\n> In general, for relevant features, like the shutter notification, I\n> would keep a fallback when it's sane to do so to ease integration of\n> new pipeline handlers.\n>\n> Thanks\n> j\n>\n> > > +\n> > > \tif (metadata.contains(controls::ScalerCrop)) {\n> > > \t\tRectangle crop = metadata.get(controls::ScalerCrop);\n> > > \t\tint32_t cropRect[] = {\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 11bdfec8d587..73e5009ac274 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -102,8 +102,7 @@ private:\n> > > \tlibcamera::PixelFormat toPixelFormat(int format) const;\n> > > \tint processControls(Camera3RequestDescriptor *descriptor);\n> > > \tstd::unique_ptr getResultMetadata(\n> > > -\t\tconst Camera3RequestDescriptor &descriptor,\n> > > -\t\tint64_t timestamp) const;\n> > > +\t\tconst Camera3RequestDescriptor &descriptor) const;\n> > >\n> > > \tunsigned int id_;\n> > > \tcamera3_device_t camera3Device_;\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 B6879BD235\n\tfor ;\n\tFri, 16 Apr 2021 09:40:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E39F46880C;\n\tFri, 16 Apr 2021 11:40:03 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD9BF605AE\n\tfor ;\n\tFri, 16 Apr 2021 11:40:02 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 16DFE200009;\n\tFri, 16 Apr 2021 09:40:01 +0000 (UTC)"],"Date":"Fri, 16 Apr 2021 11:40:40 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210416094040.sxpnetl743u7ed7b@uno.localdomain>","References":"<20210407160644.58326-1-jacopo@jmondi.org>\n\t<20210407160644.58326-4-jacopo@jmondi.org>\n\t\n\t<20210413074331.fitbqvyjh76tmmju@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210413074331.fitbqvyjh76tmmju@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Use\n\tcontrols::SensorTimestamp","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16310,"web_url":"https://patchwork.libcamera.org/comment/16310/","msgid":"","date":"2021-04-16T09:55:36","subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Use\n\tcontrols::SensorTimestamp","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Apr 16, 2021 at 11:40:40AM +0200, Jacopo Mondi wrote:\n> On Tue, Apr 13, 2021 at 09:43:31AM +0200, Jacopo Mondi wrote:\n> > On Tue, Apr 13, 2021 at 05:50:01AM +0300, Laurent Pinchart wrote:\n> > > On Wed, Apr 07, 2021 at 06:06:44PM +0200, Jacopo Mondi wrote:\n> > > > Use the controls::SensorTimestamp value to populate\n> > > > ANDROID_SENSOR_TIMESTAMP result metadata, if the Camera provides\n> > > > it.\n> > > >\n> > > > Use the same control to notify the shutter even to the camera framework\n> > >\n> > > s/even/event/\n> > >\n> > > > otherwise fall-back to the timestamp of the first completed buffer\n> > >\n> > > s/fall-back/falls back/\n> > >\n> > > > if it is not available.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_device.cpp | 33 ++++++++++++++++++++-------------\n> > > > src/android/camera_device.h | 3 +--\n> > > > 2 files changed, 21 insertions(+), 15 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index 89044efa7ebe..749fe5c3dedc 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -2019,7 +2019,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > >\n> > > > void CameraDevice::requestComplete(Request *request)\n> > > > {\n> > > > -\tconst Request::BufferMap &buffers = request->buffers();\n> > > > \tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > > > \tstd::unique_ptr resultMetadata;\n> > > > \tCamera3RequestDescriptor *descriptor =\n> > > > @@ -2034,14 +2033,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > > \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> > > > \t\t\t<< descriptor->buffers_.size() << \" streams\";\n> > > >\n> > > > -\t/*\n> > > > -\t * \\todo The timestamp used for the metadata is currently always taken\n> > > > -\t * from the first buffer (which may be the first stream) in the Request.\n> > > > -\t * It might be appropriate to return a 'correct' (as determined by\n> > > > -\t * pipeline handlers) timestamp in the Request itself.\n> > > > -\t */\n> > > > -\tuint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> > > > -\tresultMetadata = getResultMetadata(*descriptor, timestamp);\n> > > > +\tresultMetadata = getResultMetadata(*descriptor);\n> > > >\n> > > > \t/* Handle any JPEG compression. */\n> > > > \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > > > @@ -2086,6 +2078,19 @@ void CameraDevice::requestComplete(Request *request)\n> > > > \tcaptureResult.output_buffers = descriptor->buffers_.data();\n> > > >\n> > > > \tif (status == CAMERA3_BUFFER_STATUS_OK) {\n> > > > +\t\tint64_t timestamp;\n> > > > +\n> > > > +\t\tif (request->metadata().contains(controls::SensorTimestamp)) {\n> > > > +\t\t\ttimestamp = request->metadata().get(controls::SensorTimestamp);\n> > > > +\t\t} else {\n> > > > +\t\t\t/*\n> > > > +\t\t\t * Use the timestamp from the first buffer (which may be\n> > > > +\t\t\t * the first stream) in the Request if the pipeline does\n> > > > +\t\t\t * not report the sensor timestamp.\n> > > > +\t\t\t */\n> > > > +\t\t\tconst Request::BufferMap &buffers = request->buffers();\n> > > > +\t\t\ttimestamp = buffers.begin()->second->metadata().timestamp;\n> > > > +\t\t}\n> > > > \t\tnotifyShutter(descriptor->frameNumber_, timestamp);\n> > > >\n> > > > \t\tcaptureResult.partial_result = 1;\n> > > > @@ -2147,8 +2152,7 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> > > > * Produce a set of fixed result metadata.\n> > > > */\n> > > > std::unique_ptr\n> > > > -CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> > > > -\t\t\t\tint64_t timestamp) const\n> > > > +CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) const\n> > > > {\n> > > > \tconst ControlList &metadata = descriptor.request_->metadata();\n> > > > \tconst CameraMetadata &settings = descriptor.settings_;\n> > > > @@ -2274,8 +2278,6 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> > > > \tresultMetadata->addEntry(ANDROID_SENSOR_TEST_PATTERN_MODE,\n> > > > \t\t\t\t &value32, 1);\n> > > >\n> > > > -\tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);\n> > > > -\n> > > > \tvalue = ANDROID_STATISTICS_FACE_DETECT_MODE_OFF;\n> > > > \tresultMetadata->addEntry(ANDROID_STATISTICS_FACE_DETECT_MODE,\n> > > > \t\t\t\t &value, 1);\n> > > > @@ -2314,6 +2316,11 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor,\n> > > > \t\t\t\t\t &exposure, 1);\n> > > > \t}\n> > > >\n> > > > +\tif (metadata.contains(controls::SensorTimestamp)) {\n> > > > +\t\tint64_t timestamp = metadata.get(controls::SensorTimestamp);\n> > > > +\t\tresultMetadata->addEntry(ANDROID_SENSOR_TIMESTAMP, ×tamp, 1);\n> > > > +\t}\n> > >\n> > > This will break other pipeline handlers. When it comes to the Android\n> >\n> > Why do you think they break ? They might not pass CTS but they're not\n> > broken. Have I missed something ?\n> >\n> > > HAL, we care about at least UVC, rkisp1 and simple. Could you please\n> > > include patches for those ? If it's not too much extra work, addressing\n> >\n> > To report the sensor timestamp ? I could, but I know it will need to\n> > be adjusted to pass CTS.\n> \n> All-in-all, I think each pipeline handler would need to find a way\n> to correctly report the sensor timestamp, I don't think there's a\n> catch-all solution. In particular:\n> \n> - IPU3 we currently use the CIO2 timestamp, although the CIO2 driver\n> supports the FRAME_SYNC v4l2 event (and use it to update delayed\n> controls)\n> - UVC afaict I can only rely on the completed buffer timestamp\n> - RkISP1 I see it registering the V4L2_EVENT_FRAME_SYNC and the\n> pipeline uses that for delayed controls\n> - RPi does the same\n> - simple I cannot really tell as it depends on the platform in use and\n> currently there's no support for frame sync\n> \n> I'm a bit hesitant to try to add support for all the platforms without\n> being able to validate if the solution satisfies CTS, and I would\n> rather plumb the control in the pipeline once the need to do so arises\n> and we can test the implementation.\n> \n> In the meantime I cannot really tell what the implications of\n> non-reporting the sensor timestamp are. I assume it's not blocking for\n> using the HAL, but surely compliance tests won't be pleased. Should we\n> leave a default in place ? maybe using the completed buffer timestamp\n> as the current implementation does ?\n\nYou're right that we don't know :-) Not passing CTS doesn't mean nothing\nwill work, but the camera service may choke on it.\n\nYou're also right that each pipeline handler needs specific support to\ncompute precise timestamp. Without solving that issue now, I think it\nwould make sense to start with the capture timestamp, as that's readily\navailable and won't require much work. We can then improve the\nimplementation in pipeline handlers individually at a later point. This\nway we'll minimize the risk of the camera service or applications not\nbeing able to use the camera at all.\n\n> > > RPi and vimc would allow removing the fallback code in this patch.\n> >\n> > In general, for relevant features, like the shutter notification, I\n> > would keep a fallback when it's sane to do so to ease integration of\n> > new pipeline handlers.\n> >\n> > > > +\n> > > > \tif (metadata.contains(controls::ScalerCrop)) {\n> > > > \t\tRectangle crop = metadata.get(controls::ScalerCrop);\n> > > > \t\tint32_t cropRect[] = {\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index 11bdfec8d587..73e5009ac274 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -102,8 +102,7 @@ private:\n> > > > \tlibcamera::PixelFormat toPixelFormat(int format) const;\n> > > > \tint processControls(Camera3RequestDescriptor *descriptor);\n> > > > \tstd::unique_ptr getResultMetadata(\n> > > > -\t\tconst Camera3RequestDescriptor &descriptor,\n> > > > -\t\tint64_t timestamp) const;\n> > > > +\t\tconst Camera3RequestDescriptor &descriptor) const;\n> > > >\n> > > > \tunsigned int id_;\n> > > > \tcamera3_device_t camera3Device_;","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 F2F5BBD233\n\tfor ;\n\tFri, 16 Apr 2021 09:55:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6268068812;\n\tFri, 16 Apr 2021 11:55:40 +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 4091C605AE\n\tfor ;\n\tFri, 16 Apr 2021 11:55:39 +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 A75C05A5;\n\tFri, 16 Apr 2021 11:55:38 +0200 (CEST)"],"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=\"lfAIPVXe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618566938;\n\tbh=v8okH5KQd/4JCmkv/6d8CtSMbkB/yyEv6B51e8EquqM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lfAIPVXeSVkWsDCtbwp58vFmvz1n3h+ShcxnzENm+1UqQ5UApkQ6r3DztKBBP9k9m\n\tLH4JwSDsS0AHolADpAIIjBf4zgI/5LoIWLlEArVPg4U6u0PKA28MtBfrMi2l6GPdiF\n\tVvlcjyHQEqpPIrYzQUM3NYFTU+jXPyc7h/0z+0Oo=","Date":"Fri, 16 Apr 2021 12:55:36 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210407160644.58326-1-jacopo@jmondi.org>\n\t<20210407160644.58326-4-jacopo@jmondi.org>\n\t\n\t<20210413074331.fitbqvyjh76tmmju@uno.localdomain>\n\t<20210416094040.sxpnetl743u7ed7b@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210416094040.sxpnetl743u7ed7b@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 3/3] android: camera_device: Use\n\tcontrols::SensorTimestamp","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]