[{"id":14667,"web_url":"https://patchwork.libcamera.org/comment/14667/","msgid":"","date":"2021-01-21T21:23:57","subject":"Re: [libcamera-devel] [PATCH v2 8/9] android: Set result metadata\n\tand EXIF fields based on request metadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Jan 21, 2021 at 07:15:48PM +0900, Paul Elder wrote:\n> Set the following android result metadata:\n> - ANDROID_LENS_FOCAL_LENGTH\n> - ANDROID_LENS_APERTURE\n> - ANDROID_JPEG_GPS_TIMESTAMP\n> - ANDROID_JPEG_THUMBNAIL_SIZE\n> - ANDROID_JPEG_THUMBNAIL_QUALITY\n> - ANDROID_JPEG_GPS_COORDINATES\n> - ANDROID_JPEG_GPS_PROCESSING_METHOD\n> - ANDROID_JPEG_SIZE\n> - ANDROID_JPEG_QUALITY\n> - ANDROID_JPEG_ORIENTATION\n> \n> And the following EXIF fields:\n> - GPSDatestamp\n> - GPSTimestamp\n> - GPSLocation\n> - GPSLatitudeRef\n> - GPSLatitude\n> - GPSLongitudeRef\n> - GPSLongitude\n> - GPSAltitudeRef\n> - GPSAltitude\n> - GPSProcessingMethod\n> - FocalLength\n> - ExposureTime\n> - FNumber\n> - ISO\n> - Flash\n> - WhiteBalance\n> - SubsecTime\n> - SubsecTimeOriginal\n> - SubsecTimeDigitized\n> \n> Based on android request metadata.\n> \n> This allows the following CTS tests to pass:\n> - android.hardware.camera2.cts.StillCaptureTest#testFocalLengths\n> - android.hardware.camera2.cts.StillCaptureTest#testJpegExif\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v2:\n> - break out processControls and thumbnailer configuration into different\n> patches\n> - fix android metadata entry number and size\n> - other small changes\n> ---\n> src/android/camera_device.cpp | 30 ++++++--\n> src/android/camera_device.h | 5 +-\n> src/android/camera_stream.cpp | 7 +-\n> src/android/camera_stream.h | 4 +-\n> src/android/jpeg/post_processor_jpeg.cpp | 90 ++++++++++++++++++++----\n> src/android/jpeg/post_processor_jpeg.h | 3 +-\n> src/android/post_processor.h | 3 +-\n> 7 files changed, 116 insertions(+), 26 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 49af221b..1b803c92 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -296,8 +296,9 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,\n> */\n> \n> CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> -\tCamera *camera, unsigned int frameNumber, unsigned int numBuffers)\n> -\t: frameNumber_(frameNumber), numBuffers_(numBuffers)\n> +\tCamera *camera, unsigned int frameNumber, unsigned int numBuffers,\n> +\tCameraMetadata &settings)\n> +\t: frameNumber_(frameNumber), numBuffers_(numBuffers), settings_(settings)\n\nIt would be nice to split this change out, as proposed by Jacopo in the\nreview of 7/9.\n\n> {\n> \tbuffers_ = new camera3_stream_buffer_t[numBuffers];\n> \n> @@ -1691,9 +1692,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t * The descriptor and the associated memory reserved here are freed\n> \t * at request complete time.\n> \t */\n> +\t/* \\todo Handle null request settings */\n> +\tCameraMetadata settings(camera3Request->settings);\n> \tCamera3RequestDescriptor *descriptor =\n> \t\tnew Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number,\n> -\t\t\t\t\t camera3Request->num_output_buffers);\n> +\t\t\t\t\t camera3Request->num_output_buffers,\n> +\t\t\t\t\t settings);\n> \n> \tLOG(HAL, Debug) << \"Queueing Request to libcamera with \"\n> \t\t\t<< descriptor->numBuffers_ << \" HAL streams\";\n> @@ -1828,6 +1832,7 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t}\n> \n> \t\tint ret = cameraStream->process(*buffer, &mapped,\n> +\t\t\t\t\t\tdescriptor->settings_,\n> \t\t\t\t\t\tresultMetadata.get());\n> \t\tif (ret) {\n> \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> @@ -1924,14 +1929,21 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,\n> \n> \t/*\n> \t * \\todo Keep this in sync with the actual number of entries.\n> -\t * Currently: 33 entries, 75 bytes\n> +\t * Currently: 40 entries, 163 bytes\n> \t *\n> \t * Reserve more space for the JPEG metadata set by the post-processor.\n> \t * Currently: ANDROID_JPEG_SIZE (int32_t), ANDROID_JPEG_QUALITY (byte),\n> +\t * ANDROID_JPEG_GPS_COORDINATES (double x 3) = 24 bytes\n> +\t * ANDROID_JPEG_GPS_PROCESSING_METHOD (byte x 32) = 32 bytes\n> +\t * ANDROID_JPEG_GPS_TIMESTAMP (int64) = 8 bytes\n> \t * ANDROID_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.\n> +\t * ANDROID_JPEG_THUMBNAIL_QUALITY (int32 x 2) = 8 bytes\n\nDoes this consume 8 bytes ? I thought it only consumed 1.\n\n> +\t * ANDROID_JPEG_THUMBNAIL_SIZE (int32 x 2) = 8 bytes\n> +\t * ANDROID_LENS_APERTURE (float) = 4 bytes\n> +\t * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes\n> \t */\n> \tstd::unique_ptr resultMetadata =\n> -\t\tstd::make_unique(33, 75);\n> +\t\tstd::make_unique(40, 163);\n> \tif (!resultMetadata->isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to allocate static metadata\";\n> \t\treturn nullptr;\n> @@ -1996,6 +2008,14 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,\n> \tvalue = ANDROID_FLASH_STATE_UNAVAILABLE;\n> \tresultMetadata->addEntry(ANDROID_FLASH_STATE, &value, 1);\n> \n> +\tcamera_metadata_ro_entry_t entry;\n> +\tint ret = descriptor->settings_.getEntry(ANDROID_LENS_APERTURE, &entry);\n> +\tif (ret)\n> +\t\tresultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);\n> +\n> +\tfloat focal_length = 1.0;\n> +\tresultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);\n> +\n> \tvalue = ANDROID_LENS_STATE_STATIONARY;\n> \tresultMetadata->addEntry(ANDROID_LENS_STATE, &value, 1);\n> \n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index a285d0a1..111a7d8f 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -24,6 +24,7 @@\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/message.h\"\n> \n> +#include \"camera_metadata.h\"\n> #include \"camera_stream.h\"\n> #include \"camera_worker.h\"\n> #include \"jpeg/encoder.h\"\n> @@ -78,7 +79,8 @@ private:\n> \tstruct Camera3RequestDescriptor {\n> \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n> \t\t\t\t\t unsigned int frameNumber,\n> -\t\t\t\t\t unsigned int numBuffers);\n> +\t\t\t\t\t unsigned int numBuffers,\n> +\t\t\t\t\t CameraMetadata &settings);\n> \t\t~Camera3RequestDescriptor();\n> \n> \t\tuint32_t frameNumber_;\n> @@ -86,6 +88,7 @@ private:\n> \t\tcamera3_stream_buffer_t *buffers_;\n> \t\tstd::vector> frameBuffers_;\n> \t\tstd::unique_ptr request_;\n> +\t\tCameraMetadata settings_;\n> \t};\n> \n> \tstruct Camera3StreamConfiguration {\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index dba351a4..4c8020e5 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -96,12 +96,15 @@ int CameraStream::configure()\n> }\n> \n> int CameraStream::process(const libcamera::FrameBuffer &source,\n> -\t\t\t MappedCamera3Buffer *dest, CameraMetadata *metadata)\n> +\t\t\t MappedCamera3Buffer *dest,\n> +\t\t\t const CameraMetadata &requestMetadata,\n> +\t\t\t CameraMetadata *resultMetadata)\n> {\n> \tif (!postProcessor_)\n> \t\treturn 0;\n> \n> -\treturn postProcessor_->process(source, dest->maps()[0], metadata);\n> +\treturn postProcessor_->process(source, dest->maps()[0],\n> +\t\t\t\t requestMetadata, resultMetadata);\n> }\n> \n> FrameBuffer *CameraStream::getBuffer()\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index cc9d5470..298ffbf6 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -119,7 +119,9 @@ public:\n> \n> \tint configure();\n> \tint process(const libcamera::FrameBuffer &source,\n> -\t\t MappedCamera3Buffer *dest, CameraMetadata *metadata);\n> +\t\t MappedCamera3Buffer *dest,\n> +\t\t const CameraMetadata &requestMetadata,\n> +\t\t CameraMetadata *resultMetadata);\n> \tlibcamera::FrameBuffer *getBuffer();\n> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> \n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 0c1226ad..ef5e011e 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -90,17 +90,26 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> \n> int PostProcessorJpeg::process(const FrameBuffer &source,\n> \t\t\t Span destination,\n> -\t\t\t CameraMetadata *metadata)\n> +\t\t\t const CameraMetadata &requestMetadata,\n> +\t\t\t CameraMetadata *resultMetadata)\n> {\n> \tif (!encoder_)\n> \t\treturn 0;\n> \n> +\tcamera_metadata_ro_entry_t entry;\n> +\tint ret;\n> +\n> \t/* Set EXIF metadata for various tags. */\n> \tExif exif;\n> -\t/* \\todo Set Make and Model from external vendor tags. */\n> -\texif.setMake(\"libcamera\");\n> -\texif.setModel(\"cameraModel\");\n> -\texif.setOrientation(cameraDevice_->orientation());\n> +\texif.setMake(cameraDevice_->cameraMake());\n> +\texif.setModel(cameraDevice_->cameraModel());\n> +\n> +\tret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);\n> +\n> +\tconst uint32_t jpeg_orientation = ret ? *entry.data.i32 : 0;\n\ns/jpeg_orientation/jpegOrientation/\n\n> +\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n> +\texif.setOrientation(ret ? *entry.data.i32 : jpeg_orientation);\n\nIf ret != 0, this will be identical to jpeg_orientation, so I think you\ncan pass jpeg_orientation unconditionally to this function.\n\n> +\n> \texif.setSize(streamSize_);\n> \t/*\n> \t * We set the frame's EXIF timestamp as the time of encode.\n> @@ -109,10 +118,62 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \t */\n> \texif.setTimestamp(std::time(nullptr));\n> \n> -\tstd::vector thumbnail;\n> -\tgenerateThumbnail(source, &thumbnail);\n> -\tif (!thumbnail.empty())\n> -\t\texif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n> +\texif.setExposureTime(0);\n> +\tret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);\n> +\tif (ret)\n> +\t\texif.setAperture(*entry.data.f);\n> +\texif.setISO(100);\n> +\texif.setFlash(Exif::Flash::FlashNotPresent);\n> +\texif.setWhiteBalance(Exif::WhiteBalance::Auto);\n> +\n> +\texif.setSubsecTime(0);\n> +\texif.setSubsecTimeOriginal(0);\n> +\texif.setSubsecTimeDigitized(0);\n> +\n> +\texif.setFocalLength(1.0);\n> +\n> +\tret = requestMetadata.getEntry(ANDROID_JPEG_GPS_TIMESTAMP, &entry);\n> +\tif (ret) {\n> +\t\texif.setGPSDateTimestamp(*entry.data.i64);\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_GPS_TIMESTAMP,\n> +\t\t\t\t\t entry.data.i64, 1);\n> +\t}\n> +\n> +\tret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);\n> +\tif (ret) {\n> +\t\tconst int32_t *data = entry.data.i32;\n> +\t\tSize thumbnailSize = { static_cast(data[0]),\n> +\t\t\t\t static_cast(data[1]) };\n> +\n> +\t\tstd::vector thumbnail;\n\nThis can be moved within the if () { ... }.\n\n> +\t\tif (thumbnailSize != Size(0, 0)) {\n> +\t\t\tconfigureThumbnailer(thumbnailSize, thumbnailer_.pixelFormat());\n\nThis isn't quite right, configure() function takes the size and pixel\nformat of the source, not of the target. I think you could simply pass\nthe target size to PostProcessorJpeg::generateThumbnail() and\nThumbnailer::createThumbnail(), and drop\nThumbnailer::computeThumbnailSize().\n\nWould it also make sense to split the thumbnail-related part of this\npatch to a separate patch ?\n\n> +\t\t\tgenerateThumbnail(source, &thumbnail);\n> +\t\t\texif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n> +\t\t}\n> +\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);\n> +\n> +\t\t/* \\todo Use this quality as a parameter to the encoder */\n> +\t\tret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);\n> +\t\tif (ret)\n> +\t\t\tresultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, entry.data.u8, 1);\n> +\t}\n> +\n> +\tret = requestMetadata.getEntry(ANDROID_JPEG_GPS_COORDINATES, &entry);\n> +\tif (ret) {\n> +\t\texif.setGPSLocation(entry.data.d);\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_GPS_COORDINATES,\n> +\t\t\t\t\t entry.data.d, 3);\n> +\t}\n> +\n> +\tret = requestMetadata.getEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD, &entry);\n> +\tif (ret) {\n> +\t\tstd::string method(entry.data.u8, entry.data.u8 + 32);\n\nmethod.size() will return 32, the string won't be truncated to the first\n\\0. Is that desired ?\n\nIs this guaranteed to be 32 bytes long, or should you use entry.count as\nthe size ?\n\n> +\t\texif.setGPSMethod(method);\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,\n> +\t\t\t\t\t entry.data.u8, 32);\n\nSame here.\n\n> +\t}\n> \n> \tif (exif.generate() != 0)\n> \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> @@ -143,13 +204,12 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \tblob->jpeg_size = jpeg_size;\n> \n> \t/* Update the JPEG result Metadata. */\n> -\tmetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n> -\n> -\tconst uint32_t jpeg_quality = 95;\n> -\tmetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n> +\tresultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);\n> \n> -\tconst uint32_t jpeg_orientation = 0;\n> -\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n> +\t/* \\todo Configure JPEG encoder with this */\n> +\tret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);\n> +\tconst uint32_t jpeg_quality = ret ? *entry.data.u8 : 95;\n> +\tresultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n\nUnless I'm mistaken, it should be just a matter of passing the quality\nto Encoder::encode() (with a default of 95 maybe), dropping the quality_\nmember of EncoderLibJpeg and moving the jpeg_set_quality() call to\nEncoderLibJpeg::encode(). This can be done on top of this series, but it\nwould be nice to address it soon before we forget about it.\n\n> \n> \treturn 0;\n> }\n> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h\n> index c545c29c..d07c9fe5 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -26,7 +26,8 @@ public:\n> \t\t const libcamera::StreamConfiguration &outcfg) override;\n> \tint process(const libcamera::FrameBuffer &source,\n> \t\t libcamera::Span destination,\n> -\t\t CameraMetadata *metadata) override;\n> +\t\t const CameraMetadata &requestMetadata,\n> +\t\t CameraMetadata *resultMetadata) override;\n> \n> private:\n> \tvoid generateThumbnail(const libcamera::FrameBuffer &source,\n> diff --git a/src/android/post_processor.h b/src/android/post_processor.h\n> index e0f91880..bda93bb4 100644\n> --- a/src/android/post_processor.h\n> +++ b/src/android/post_processor.h\n> @@ -22,7 +22,8 @@ public:\n> \t\t\t const libcamera::StreamConfiguration &outCfg) = 0;\n> \tvirtual int process(const libcamera::FrameBuffer &source,\n> \t\t\t libcamera::Span destination,\n> -\t\t\t CameraMetadata *metadata) = 0;\n> +\t\t\t const CameraMetadata &requestMetadata,\n> +\t\t\t CameraMetadata *resultMetadata) = 0;\n> };\n> \n> #endif /* __ANDROID_POST_PROCESSOR_H__ */","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 1F952BD808\n\tfor ;\n\tThu, 21 Jan 2021 21:24:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 85F686821B;\n\tThu, 21 Jan 2021 22:24:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 587AF68204\n\tfor ;\n\tThu, 21 Jan 2021 22:24:16 +0100 (CET)","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 BF6F250E;\n\tThu, 21 Jan 2021 22:24:15 +0100 (CET)"],"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=\"UkAZy++k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611264256;\n\tbh=Ygwc2HirgJRWEDKzW+AOa4kUsbMGPaoBCG71wKFouhk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UkAZy++kLRClRzUJNEG6soLusWU7IwBMLBbaQU2iGq0BICI7cfayE5FpkQuJ62p17\n\topWjcN6Mxi9ssePB367in9XA3Yb/8MzuKldCqZ/3d+Csn0ltrO0/XYXkA1ZOjUF6WU\n\tAQzzXRrRS7Uz55yScQyWpEHBZN3TpzBDIYRXabDs=","Date":"Thu, 21 Jan 2021 23:23:57 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210121101549.134574-1-paul.elder@ideasonboard.com>\n\t<20210121101549.134574-9-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210121101549.134574-9-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 8/9] android: Set result metadata\n\tand EXIF fields based on request metadata","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\" "}}]