[{"id":14551,"web_url":"https://patchwork.libcamera.org/comment/14551/","msgid":"<20210115154812.hbpc5lfgciw6xohq@uno.localdomain>","date":"2021-01-15T15:48:12","subject":"Re: [libcamera-devel] [PATCH 6/6] android: Set result metadata and\n\tEXIF fields based on request metadata","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Thu, Jan 14, 2021 at 07:40:35PM +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\\o/\n\n>\n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n>  src/android/camera_device.cpp            |  27 +++++-\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 | 116 +++++++++++++++++++----\n>  src/android/jpeg/post_processor_jpeg.h   |   5 +-\n>  src/android/jpeg/thumbnailer.h           |   1 +\n>  src/android/post_processor.h             |   3 +-\n>  8 files changed, 136 insertions(+), 32 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ed47c7cd..8d697080 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>  {\n>  \tbuffers_ = new camera3_stream_buffer_t[numBuffers];\n>\n> @@ -1700,9 +1701,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 requestMetadata(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     requestMetadata);\n\nAs you use settings, I would name it settings here as well\n\n>\n>  \tLOG(HAL, Debug) << \"Queueing Request to libcamera with \"\n>  \t\t\t<< descriptor->numBuffers_ << \" HAL streams\";\n> @@ -1815,6 +1819,9 @@ void CameraDevice::requestComplete(Request *request)\n>  \t\tCameraStream *cameraStream =\n>  \t\t\tstatic_cast<CameraStream *>(descriptor->buffers_[i].stream->priv);\n>\n> +\t\tfloat focal_length = 1.0;\n> +\t\tresultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);\n> +\n\nUps, result metadata are procesed in getResultMetadata() and if you\nadd an entry you need to expand the metadata pack size. Also.. see\nbelow..\n\n>  \t\tif (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n>  \t\t\tcontinue;\n>\n> @@ -1837,6 +1844,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\nI would move:\n1) Adding JPEG tags to EXIF\n2) Filling in the result metadata\n\nto 2 different patches\n\nHowever, I'm a bit skeptic about this change.\nYou pass to the JPEG encode the settings the application has\nrequested. The actual process should go through the pipeline handler,\nso that you should be able to find all the required information in\nlibcamera::Request::metadata()\n\nThis requires several parts:\n1) Define a libcamera (draft) control to be able to translate the\nAndroid metadata to a control the libcamera pipeline handler can\nconsume\n2) Set the libcamera control in the Request's metadata\n3) Parse the control value in the completion handler here.\n\nYou can see how (parts of) this process is handled for draft::PipelineDepth.\n\nHowever, it's not trivial to make all the pieces fit together. I have\na series of patches I still have to send out that implement exactly\nthis for SCALER_CROP_REGION (except for the fact no ad-hoc property\nneeds to be defined, as we already have\nlibcamera::controls::ScalerCrop)\n\nI'm not sure if we should either rush and take this big shortcut here\nto provide the lens info to the post-processor, or rather hard-code it\nthere with a big \\todo, or maybe wait until we close the loop.. I\nwould go for the second option, but I would like to know Laurent's\nopinion here.\n\n>  \t\t\t\t\t\tresultMetadata.get());\n>  \t\tif (ret) {\n>  \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> @@ -1933,14 +1941,23 @@ 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: 41 entries, 187 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_ORIENTATION (int32_t) = 3 entries, 9 bytes.\n\nCan you move the additional JPEG sizes to the end ?\n\n        = 10 entries, 92 bytes\n\nHowever I count:\n        41 - 33 = +8\n        We have 10 JPEG metadata so I assume the additional one is for\n        LENS_FOCAL_LENGTH you add here above\n\n        187 - 75 = 112\n        I count 92 bytes for JPEG + 4 for LENS_FOCAL_LENGHT\n\n        Where do the additional 16 bytes come from ?\n\n> +\t * ANDROID_JPEG_THUMBNAIL_SIZE (int32 x 2) = 8 bytes\n> +\t * ANDROID_JPEG_THUMBNAIL_QUALITY (int32 x 2) = 8 bytes\n> +\t * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes\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_LENS_APERTURE (float) = 4 bytes\n> +\t *\n> +\t * add coordinates again?\n\n\nNot sure I get this todo :)\n\n>  \t */\n>  \tstd::unique_ptr<CameraMetadata> resultMetadata =\n> -\t\tstd::make_unique<CameraMetadata>(33, 75);\n> +\t\tstd::make_unique<CameraMetadata>(41, 187);\n>  \tif (!resultMetadata->isValid()) {\n>  \t\tLOG(HAL, Error) << \"Failed to allocate static metadata\";\n>  \t\treturn nullptr;\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<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;\n>  \t\tstd::unique_ptr<CaptureRequest> request_;\n> +\t\tCameraMetadata settings_;\n>  \t};\n>\n>  \tstruct Camera3StreamConfiguration {\n\nThe part below should go to a separate patch imho\n\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 436a50f8..13ad3777 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -25,6 +25,21 @@ PostProcessorJpeg::PostProcessorJpeg(CameraDevice *const device)\n>  {\n>  }\n>\n> +int PostProcessorJpeg::configureThumbnailer(const Size &size,\n> +\t\t\t\t\t    const PixelFormat &pixelFormat)\n> +{\n> +\tthumbnailer_.configure(size, pixelFormat);\n> +\tStreamConfiguration thCfg;\n> +\tthCfg.size = thumbnailer_.size();\n> +\tthCfg.pixelFormat = pixelFormat;\n> +\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> +\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n>  int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n>  \t\t\t\t const StreamConfiguration &outCfg)\n>  {\n> @@ -40,16 +55,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n>\n>  \tstreamSize_ = outCfg.size;\n>\n> -\tthumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> -\tStreamConfiguration thCfg = inCfg;\n> -\tthCfg.size = thumbnailer_.size();\n> -\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> -\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> -\t\treturn -EINVAL;\n> -\t}\n> +\tint ret = configureThumbnailer(inCfg.size, inCfg.pixelFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n>\n>  \tencoder_ = std::make_unique<EncoderLibJpeg>();\n> -\n>  \treturn encoder_->configure(inCfg);\n\nUnrelated changes ? Should they go to a third patch ?\n\nI'll review the rest once separated to a new patch\n\nThanks\n   j\n\n>  }\n>\n> @@ -80,17 +90,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n>\n>  int PostProcessorJpeg::process(const FrameBuffer &source,\n>  \t\t\t       Span<uint8_t> 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\nIf you intend to handle the ANDROID_JPEG_ORIENTATION control (do you\nneed this?) I think you should combine it with the camera orientation.\n\n> +\texif.setOrientation(ret ? cameraDevice_->orientation() : *entry.data.i32);\n>  \texif.setSize(streamSize_);\n>  \t/*\n>  \t * We set the frame's EXIF timestamp as the time of encode.\n> @@ -99,11 +114,68 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n>  \t */\n>  \texif.setTimestamp(std::time(nullptr));\n>\n> -\tstd::vector<unsigned char> thumbnail;\n> -\tgenerateThumbnail(source, &thumbnail);\n> -\tif (!thumbnail.empty())\n> +\texif.setExposureTime(0);\n> +\tret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);\n> +\tif (!ret) {\n> +\t\texif.setAperture(*entry.data.f);\n> +\t\tresultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);\n> +\t}\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> +\tfloat focal_length = 1.0;\n> +\texif.setFocalLength(focal_length);\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\t/* \\todo Handle non-matching aspect ratios */\n> +\t\t/* \\todo Handle non-zero orientations */\n> +\t\tconst int32_t *data = entry.data.i32;\n> +\t\tSize thumbnailSize = { static_cast<uint32_t>(data[0]),\n> +\t\t\t\t       static_cast<uint32_t>(data[1]) };\n> +\n> +\t\tstd::vector<unsigned char> thumbnail;\n> +\t\tif (thumbnailSize != Size(0, 0)) {\n> +\t\t\tconfigureThumbnailer(thumbnailSize, thumbnailer_.pixelFormat());\n> +\t\t\tgenerateThumbnail(source, &thumbnail);\n> +\t\t}\n>  \t\texif.setThumbnail(thumbnail, Exif::Compression::JPEG);\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> +\t\texif.setGPSMethod(method);\n> +\t\tresultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,\n> +\t\t\t\t\t entry.data.u8, 32);\n> +\t}\n> +\n>  \tif (exif.generate() != 0)\n>  \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n>\n> @@ -133,13 +205,15 @@ 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> +\tresultMetadata->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> +\tret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);\n> +\tconst uint32_t jpeg_quality = ret ? 95 : *entry.data.u8;\n> +\tresultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n>\n> -\tconst uint32_t jpeg_orientation = 0;\n> -\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n> +\tret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);\n> +\tconst uint32_t jpeg_orientation = ret ? 0 : *entry.data.i32;\n> +\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\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 5afa831c..d07c9fe5 100644\n> --- a/src/android/jpeg/post_processor_jpeg.h\n> +++ b/src/android/jpeg/post_processor_jpeg.h\n> @@ -26,11 +26,14 @@ public:\n>  \t\t      const libcamera::StreamConfiguration &outcfg) override;\n>  \tint process(const libcamera::FrameBuffer &source,\n>  \t\t    libcamera::Span<uint8_t> 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>  \t\t\t       std::vector<unsigned char> *thumbnail);\n> +\tint configureThumbnailer(const libcamera::Size &size,\n> +\t\t\t\t const libcamera::PixelFormat &pixelFormat);\n>\n>  \tCameraDevice *const cameraDevice_;\n>  \tstd::unique_ptr<Encoder> encoder_;\n> diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> index 98f11833..f393db47 100644\n> --- a/src/android/jpeg/thumbnailer.h\n> +++ b/src/android/jpeg/thumbnailer.h\n> @@ -22,6 +22,7 @@ public:\n>  \tvoid createThumbnail(const libcamera::FrameBuffer &source,\n>  \t\t\t     std::vector<unsigned char> *dest);\n>  \tconst libcamera::Size &size() const { return targetSize_; }\n> +\tconst libcamera::PixelFormat &pixelFormat() const { return pixelFormat_; }\n>\n>  private:\n>  \tlibcamera::Size computeThumbnailSize() const;\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<uint8_t> 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__ */\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6B30DC3383\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Jan 2021 15:47:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F3886680EE;\n\tFri, 15 Jan 2021 16:47:55 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 492A7680D1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Jan 2021 16:47:54 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id B17851BF20C;\n\tFri, 15 Jan 2021 15:47:53 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 15 Jan 2021 16:48:12 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20210115154812.hbpc5lfgciw6xohq@uno.localdomain>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210114104035.302968-7-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 6/6] android: Set result metadata and\n\tEXIF fields based on request metadata","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14558,"web_url":"https://patchwork.libcamera.org/comment/14558/","msgid":"<YAHvMHl0TAbWUpnZ@pendragon.ideasonboard.com>","date":"2021-01-15T19:38:24","subject":"Re: [libcamera-devel] [PATCH 6/6] android: Set result metadata and\n\tEXIF 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 Fri, Jan 15, 2021 at 04:48:12PM +0100, Jacopo Mondi wrote:\n> On Thu, Jan 14, 2021 at 07:40:35PM +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\nCan you split this in multiple patches ? Review is difficult. You can\nhave one patch that sets all the result metadata and Exif tags that do\nnot interact with any other component, and one patch per change for all\nother changes. For instance ANDROID_JPEG_THUMBNAIL_SIZE requires\nconfiguring the thumbnailer accordingly, and should be in a patch of its\nown.\n\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> \\o/\n>\n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > ---\n> >  src/android/camera_device.cpp            |  27 +++++-\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 | 116 +++++++++++++++++++----\n> >  src/android/jpeg/post_processor_jpeg.h   |   5 +-\n> >  src/android/jpeg/thumbnailer.h           |   1 +\n> >  src/android/post_processor.h             |   3 +-\n> >  8 files changed, 136 insertions(+), 32 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ed47c7cd..8d697080 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> >  {\n> >  \tbuffers_ = new camera3_stream_buffer_t[numBuffers];\n> >\n> > @@ -1700,9 +1701,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\nCan it happen ? If so it should be handled as part of this series or\nimmediately on top, not left for later.\n\n> > +\tCameraMetadata requestMetadata(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     requestMetadata);\n>\n> As you use settings, I would name it settings here as well\n>\n> >\n> >  \tLOG(HAL, Debug) << \"Queueing Request to libcamera with \"\n> >  \t\t\t<< descriptor->numBuffers_ << \" HAL streams\";\n> > @@ -1815,6 +1819,9 @@ void CameraDevice::requestComplete(Request *request)\n> >  \t\tCameraStream *cameraStream =\n> >  \t\t\tstatic_cast<CameraStream *>(descriptor->buffers_[i].stream->priv);\n> >\n> > +\t\tfloat focal_length = 1.0;\n> > +\t\tresultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);\n> > +\n>\n> Ups, result metadata are procesed in getResultMetadata() and if you\n> add an entry you need to expand the metadata pack size. Also.. see\n> below..\n>\n> >  \t\tif (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> >  \t\t\tcontinue;\n> >\n> > @@ -1837,6 +1844,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>\n> I would move:\n> 1) Adding JPEG tags to EXIF\n> 2) Filling in the result metadata\n>\n> to 2 different patches\n>\n> However, I'm a bit skeptic about this change.\n> You pass to the JPEG encode the settings the application has\n> requested. The actual process should go through the pipeline handler,\n> so that you should be able to find all the required information in\n> libcamera::Request::metadata()\n>\n> This requires several parts:\n> 1) Define a libcamera (draft) control to be able to translate the\n> Android metadata to a control the libcamera pipeline handler can\n> consume\n> 2) Set the libcamera control in the Request's metadata\n> 3) Parse the control value in the completion handler here.\n>\n> You can see how (parts of) this process is handled for draft::PipelineDepth.\n>\n> However, it's not trivial to make all the pieces fit together. I have\n> a series of patches I still have to send out that implement exactly\n> this for SCALER_CROP_REGION (except for the fact no ad-hoc property\n> needs to be defined, as we already have\n> libcamera::controls::ScalerCrop)\n>\n> I'm not sure if we should either rush and take this big shortcut here\n> to provide the lens info to the post-processor, or rather hard-code it\n> there with a big \\todo, or maybe wait until we close the loop.. I\n> would go for the second option, but I would like to know Laurent's\n> opinion here.\n\nThere are Android metadata that are meant only for the HAL, such as all\nthe JPEG-related information as we don't handle JPEG encoding in\nlibcamera. Passing the request settings to the post-processor does make\nsense.\n\nLooking at the code below, the only metadata entry that needs to be\nhandled by libcamera is the lens aperture. Given that we support a\nsingle aperture at the moment (we report a single entry in\nANDROID_LENS_INFO_AVAILABLE_APERTURES), hardcoding ANDROID_LENS_APERTURE\nmakes sense, we don't need to define libcamera controls yet. However,\nsetting ANDROID_LENS_APERTURE doesn't belong to this patch (see below\nfor more comments).\n\nSame for ANDROID_LENS_FOCAL_LENGTH, we can hardcode it for now as we\nhave a fixed focus lens, but it should go to getResultMetadata() as\nyou've mentioned.\n\n> >  \t\t\t\t\t\tresultMetadata.get());\n> >  \t\tif (ret) {\n> >  \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > @@ -1933,14 +1941,23 @@ 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: 41 entries, 187 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_ORIENTATION (int32_t) = 3 entries, 9 bytes.\n>\n> Can you move the additional JPEG sizes to the end ?\n>\n>         = 10 entries, 92 bytes\n>\n> However I count:\n>         41 - 33 = +8\n>         We have 10 JPEG metadata so I assume the additional one is for\n>         LENS_FOCAL_LENGTH you add here above\n>\n>         187 - 75 = 112\n>         I count 92 bytes for JPEG + 4 for LENS_FOCAL_LENGHT\n>\n>         Where do the additional 16 bytes come from ?\n>\n> > +\t * ANDROID_JPEG_THUMBNAIL_SIZE (int32 x 2) = 8 bytes\n> > +\t * ANDROID_JPEG_THUMBNAIL_QUALITY (int32 x 2) = 8 bytes\n> > +\t * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes\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_LENS_APERTURE (float) = 4 bytes\n> > +\t *\n> > +\t * add coordinates again?\n>\n> Not sure I get this todo :)\n>\n> >  \t */\n> >  \tstd::unique_ptr<CameraMetadata> resultMetadata =\n> > -\t\tstd::make_unique<CameraMetadata>(33, 75);\n> > +\t\tstd::make_unique<CameraMetadata>(41, 187);\n> >  \tif (!resultMetadata->isValid()) {\n> >  \t\tLOG(HAL, Error) << \"Failed to allocate static metadata\";\n> >  \t\treturn nullptr;\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<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;\n> >  \t\tstd::unique_ptr<CaptureRequest> request_;\n> > +\t\tCameraMetadata settings_;\n> >  \t};\n> >\n> >  \tstruct Camera3StreamConfiguration {\n>\n> The part below should go to a separate patch imho\n>\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 436a50f8..13ad3777 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -25,6 +25,21 @@ PostProcessorJpeg::PostProcessorJpeg(CameraDevice *const device)\n> >  {\n> >  }\n> >\n> > +int PostProcessorJpeg::configureThumbnailer(const Size &size,\n> > +\t\t\t\t\t    const PixelFormat &pixelFormat)\n> > +{\n> > +\tthumbnailer_.configure(size, pixelFormat);\n> > +\tStreamConfiguration thCfg;\n> > +\tthCfg.size = thumbnailer_.size();\n> > +\tthCfg.pixelFormat = pixelFormat;\n> > +\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> > +\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> >  int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> >  \t\t\t\t const StreamConfiguration &outCfg)\n> >  {\n> > @@ -40,16 +55,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> >\n> >  \tstreamSize_ = outCfg.size;\n> >\n> > -\tthumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> > -\tStreamConfiguration thCfg = inCfg;\n> > -\tthCfg.size = thumbnailer_.size();\n> > -\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> > -\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > +\tint ret = configureThumbnailer(inCfg.size, inCfg.pixelFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> >\n> >  \tencoder_ = std::make_unique<EncoderLibJpeg>();\n> > -\n> >  \treturn encoder_->configure(inCfg);\n>\n> Unrelated changes ? Should they go to a third patch ?\n\nThe introduction of PostProcessorJpeg::configureThumbnailer(), if\ndesired, should be split to a patch of its own, yes.\n\n> I'll review the rest once separated to a new patch\n>\n> >  }\n> >\n> > @@ -80,17 +90,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> >\n> >  int PostProcessorJpeg::process(const FrameBuffer &source,\n> >  \t\t\t       Span<uint8_t> 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> If you intend to handle the ANDROID_JPEG_ORIENTATION control (do you\n> need this?) I think you should combine it with the camera orientation.\n>\n> > +\texif.setOrientation(ret ? cameraDevice_->orientation() : *entry.data.i32);\n> >  \texif.setSize(streamSize_);\n> >  \t/*\n> >  \t * We set the frame's EXIF timestamp as the time of encode.\n> > @@ -99,11 +114,68 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> >  \t */\n> >  \texif.setTimestamp(std::time(nullptr));\n> >\n> > -\tstd::vector<unsigned char> thumbnail;\n> > -\tgenerateThumbnail(source, &thumbnail);\n> > -\tif (!thumbnail.empty())\n> > +\texif.setExposureTime(0);\n> > +\tret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);\n> > +\tif (!ret) {\n> > +\t\texif.setAperture(*entry.data.f);\n> > +\t\tresultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);\n\nThis doesn't belong to PostProcessorJpeg::process(), only\nexif.setAperture() belongs here. ANDROID_LENS_APERTURE should be set in\nCameraDevice::getResultMetadata().\n\n> > +\t}\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> > +\tfloat focal_length = 1.0;\n> > +\texif.setFocalLength(focal_length);\n\n\texif.setFocalLength(1.0);\n\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\nAs part as the rework of the thumbnail generation, you need to report\nadditional sizes in ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.\n\n> > +\tret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);\n> > +\tif (!ret) {\n> > +\t\t/* \\todo Handle non-matching aspect ratios */\n> > +\t\t/* \\todo Handle non-zero orientations */\n\nIs this required ? As we handle the orientation by setting it in the\nExif tags, we don't need to rotate the thumbnail, is there anything else\nto handle ?\n\n> > +\t\tconst int32_t *data = entry.data.i32;\n> > +\t\tSize thumbnailSize = { static_cast<uint32_t>(data[0]),\n> > +\t\t\t\t       static_cast<uint32_t>(data[1]) };\n> > +\n> > +\t\tstd::vector<unsigned char> thumbnail;\n> > +\t\tif (thumbnailSize != Size(0, 0)) {\n> > +\t\t\tconfigureThumbnailer(thumbnailSize, thumbnailer_.pixelFormat());\n> > +\t\t\tgenerateThumbnail(source, &thumbnail);\n> > +\t\t}\n> >  \t\texif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n\nYou shouldn't set a thumbnail at all if the size is { 0, 0 }.\n\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> > +\t\texif.setGPSMethod(method);\n> > +\t\tresultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,\n> > +\t\t\t\t\t entry.data.u8, 32);\n> > +\t}\n> > +\n> >  \tif (exif.generate() != 0)\n> >  \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> >\n> > @@ -133,13 +205,15 @@ 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> > +\tresultMetadata->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> > +\tret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);\n> > +\tconst uint32_t jpeg_quality = ret ? 95 : *entry.data.u8;\n> > +\tresultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n\nIf we start acting on ANDROID_JPEG_QUALITY, we should really take it\ninto account and configure the JPEG encoder accordingly. This can be\ndone on top.\n\n> >\n> > -\tconst uint32_t jpeg_orientation = 0;\n> > -\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n> > +\tret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);\n> > +\tconst uint32_t jpeg_orientation = ret ? 0 : *entry.data.i32;\n> > +\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\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 5afa831c..d07c9fe5 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.h\n> > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > @@ -26,11 +26,14 @@ public:\n> >  \t\t      const libcamera::StreamConfiguration &outcfg) override;\n> >  \tint process(const libcamera::FrameBuffer &source,\n> >  \t\t    libcamera::Span<uint8_t> destination,\n> > -\t\t    CameraMetadata *metadata) override;\n> > +\t\t    const CameraMetadata &requestMetadata,\n> > +\t\t    CameraMetadata *resultMetadata) override;\n\nI'd split this change and the similar one below to a separate patch, it\nwill make this patch easier to review.\n\n> >\n> >  private:\n> >  \tvoid generateThumbnail(const libcamera::FrameBuffer &source,\n> >  \t\t\t       std::vector<unsigned char> *thumbnail);\n> > +\tint configureThumbnailer(const libcamera::Size &size,\n> > +\t\t\t\t const libcamera::PixelFormat &pixelFormat);\n> >\n> >  \tCameraDevice *const cameraDevice_;\n> >  \tstd::unique_ptr<Encoder> encoder_;\n> > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> > index 98f11833..f393db47 100644\n> > --- a/src/android/jpeg/thumbnailer.h\n> > +++ b/src/android/jpeg/thumbnailer.h\n> > @@ -22,6 +22,7 @@ public:\n> >  \tvoid createThumbnail(const libcamera::FrameBuffer &source,\n> >  \t\t\t     std::vector<unsigned char> *dest);\n> >  \tconst libcamera::Size &size() const { return targetSize_; }\n> > +\tconst libcamera::PixelFormat &pixelFormat() const { return pixelFormat_; }\n> >\n> >  private:\n> >  \tlibcamera::Size computeThumbnailSize() const;\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<uint8_t> 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__ */\n\n--\nRegards,\n\nLaurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 F400CBD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Jan 2021 19:38:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 859C3680FF;\n\tFri, 15 Jan 2021 20:38:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 33FE2680EF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Jan 2021 20:38:42 +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 9090A58B;\n\tFri, 15 Jan 2021 20:38:41 +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=\"cI4E7jx4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610739521;\n\tbh=V0mcv2efMWjbwPmPCgnZ1spV0Djvb8ae63HLijoYEfE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cI4E7jx4u1oDFRy9kYXudMSh81aSz7P6D6WrWtNllZof1gc+JjNKsYB9lZ/T9XpAw\n\tHVC+b38lubw+vdx7fowDqgDvkyI0CWv6xiHO616kfFNu/HBpY9BVI991JNWI2op9C7\n\tBa3BDdxmrcf3rEL9aUIgJNNfM+k6SyeQsubSdZIg=","Date":"Fri, 15 Jan 2021 21:38:24 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YAHvMHl0TAbWUpnZ@pendragon.ideasonboard.com>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-7-paul.elder@ideasonboard.com>\n\t<20210115154812.hbpc5lfgciw6xohq@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210115154812.hbpc5lfgciw6xohq@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 6/6] android: Set result metadata and\n\tEXIF fields based on request metadata","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14568,"web_url":"https://patchwork.libcamera.org/comment/14568/","msgid":"<20210118093541.fqgqk2g4qmpsifb2@uno.localdomain>","date":"2021-01-18T09:35:41","subject":"Re: [libcamera-devel] [PATCH 6/6] android: Set result metadata and\n\tEXIF fields based on request metadata","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Jan 15, 2021 at 09:38:24PM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n>\n> Thank you for the patch.\n>\n> > >  \t\tint ret = cameraStream->process(*buffer, &mapped,\n> > > +\t\t\t\t\t\tdescriptor->settings_,\n> >\n> > I would move:\n> > 1) Adding JPEG tags to EXIF\n> > 2) Filling in the result metadata\n> >\n> > to 2 different patches\n> >\n> > However, I'm a bit skeptic about this change.\n> > You pass to the JPEG encode the settings the application has\n> > requested. The actual process should go through the pipeline handler,\n> > so that you should be able to find all the required information in\n> > libcamera::Request::metadata()\n> >\n> > This requires several parts:\n> > 1) Define a libcamera (draft) control to be able to translate the\n> > Android metadata to a control the libcamera pipeline handler can\n> > consume\n> > 2) Set the libcamera control in the Request's metadata\n> > 3) Parse the control value in the completion handler here.\n> >\n> > You can see how (parts of) this process is handled for draft::PipelineDepth.\n> >\n> > However, it's not trivial to make all the pieces fit together. I have\n> > a series of patches I still have to send out that implement exactly\n> > this for SCALER_CROP_REGION (except for the fact no ad-hoc property\n> > needs to be defined, as we already have\n> > libcamera::controls::ScalerCrop)\n> >\n> > I'm not sure if we should either rush and take this big shortcut here\n> > to provide the lens info to the post-processor, or rather hard-code it\n> > there with a big \\todo, or maybe wait until we close the loop.. I\n> > would go for the second option, but I would like to know Laurent's\n> > opinion here.\n>\n> There are Android metadata that are meant only for the HAL, such as all\n> the JPEG-related information as we don't handle JPEG encoding in\n> libcamera. Passing the request settings to the post-processor does make\n> sense.\n>\n> Looking at the code below, the only metadata entry that needs to be\n> handled by libcamera is the lens aperture. Given that we support a\n> single aperture at the moment (we report a single entry in\n> ANDROID_LENS_INFO_AVAILABLE_APERTURES), hardcoding ANDROID_LENS_APERTURE\n> makes sense, we don't need to define libcamera controls yet. However,\n> setting ANDROID_LENS_APERTURE doesn't belong to this patch (see below\n> for more comments).\n>\n> Same for ANDROID_LENS_FOCAL_LENGTH, we can hardcode it for now as we\n> have a fixed focus lens, but it should go to getResultMetadata() as\n> you've mentioned.\n>\n\nAgreed with the fixed values.\n\n> > >  \t\t\t\t\t\tresultMetadata.get());\n> > >  \t\tif (ret) {\n> > >  \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > > @@ -1933,14 +1941,23 @@ 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: 41 entries, 187 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_ORIENTATION (int32_t) = 3 entries, 9 bytes.\n> >\n> > Can you move the additional JPEG sizes to the end ?\n> >\n> >         = 10 entries, 92 bytes\n> >\n> > However I count:\n> >         41 - 33 = +8\n> >         We have 10 JPEG metadata so I assume the additional one is for\n> >         LENS_FOCAL_LENGTH you add here above\n> >\n> >         187 - 75 = 112\n> >         I count 92 bytes for JPEG + 4 for LENS_FOCAL_LENGHT\n> >\n> >         Where do the additional 16 bytes come from ?\n> >\n> > > +\t * ANDROID_JPEG_THUMBNAIL_SIZE (int32 x 2) = 8 bytes\n> > > +\t * ANDROID_JPEG_THUMBNAIL_QUALITY (int32 x 2) = 8 bytes\n> > > +\t * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes\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_LENS_APERTURE (float) = 4 bytes\n> > > +\t *\n> > > +\t * add coordinates again?\n> >\n> > Not sure I get this todo :)\n> >\n> > >  \t */\n> > >  \tstd::unique_ptr<CameraMetadata> resultMetadata =\n> > > -\t\tstd::make_unique<CameraMetadata>(33, 75);\n> > > +\t\tstd::make_unique<CameraMetadata>(41, 187);\n> > >  \tif (!resultMetadata->isValid()) {\n> > >  \t\tLOG(HAL, Error) << \"Failed to allocate static metadata\";\n> > >  \t\treturn nullptr;\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<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;\n> > >  \t\tstd::unique_ptr<CaptureRequest> request_;\n> > > +\t\tCameraMetadata settings_;\n> > >  \t};\n> > >\n> > >  \tstruct Camera3StreamConfiguration {\n> >\n> > The part below should go to a separate patch imho\n> >\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 436a50f8..13ad3777 100644\n> > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > @@ -25,6 +25,21 @@ PostProcessorJpeg::PostProcessorJpeg(CameraDevice *const device)\n> > >  {\n> > >  }\n> > >\n> > > +int PostProcessorJpeg::configureThumbnailer(const Size &size,\n> > > +\t\t\t\t\t    const PixelFormat &pixelFormat)\n> > > +{\n> > > +\tthumbnailer_.configure(size, pixelFormat);\n> > > +\tStreamConfiguration thCfg;\n> > > +\tthCfg.size = thumbnailer_.size();\n> > > +\tthCfg.pixelFormat = pixelFormat;\n> > > +\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> > > +\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > >  int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > >  \t\t\t\t const StreamConfiguration &outCfg)\n> > >  {\n> > > @@ -40,16 +55,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > >\n> > >  \tstreamSize_ = outCfg.size;\n> > >\n> > > -\tthumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> > > -\tStreamConfiguration thCfg = inCfg;\n> > > -\tthCfg.size = thumbnailer_.size();\n> > > -\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> > > -\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> > > -\t\treturn -EINVAL;\n> > > -\t}\n> > > +\tint ret = configureThumbnailer(inCfg.size, inCfg.pixelFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > >\n> > >  \tencoder_ = std::make_unique<EncoderLibJpeg>();\n> > > -\n> > >  \treturn encoder_->configure(inCfg);\n> >\n> > Unrelated changes ? Should they go to a third patch ?\n>\n> The introduction of PostProcessorJpeg::configureThumbnailer(), if\n> desired, should be split to a patch of its own, yes.\n>\n\nI mean the empy line, but yeah, the newly introduce function even more\n\n> > I'll review the rest once separated to a new patch\n> >\n> > >  }\n> > >\n> > > @@ -80,17 +90,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > >\n> > >  int PostProcessorJpeg::process(const FrameBuffer &source,\n> > >  \t\t\t       Span<uint8_t> 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> > If you intend to handle the ANDROID_JPEG_ORIENTATION control (do you\n> > need this?) I think you should combine it with the camera orientation.\n> >\n> > > +\texif.setOrientation(ret ? cameraDevice_->orientation() : *entry.data.i32);\n> > >  \texif.setSize(streamSize_);\n> > >  \t/*\n> > >  \t * We set the frame's EXIF timestamp as the time of encode.\n> > > @@ -99,11 +114,68 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > >  \t */\n> > >  \texif.setTimestamp(std::time(nullptr));\n> > >\n> > > -\tstd::vector<unsigned char> thumbnail;\n> > > -\tgenerateThumbnail(source, &thumbnail);\n> > > -\tif (!thumbnail.empty())\n> > > +\texif.setExposureTime(0);\n> > > +\tret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);\n> > > +\tif (!ret) {\n> > > +\t\texif.setAperture(*entry.data.f);\n> > > +\t\tresultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);\n>\n> This doesn't belong to PostProcessorJpeg::process(), only\n> exif.setAperture() belongs here. ANDROID_LENS_APERTURE should be set in\n> CameraDevice::getResultMetadata().\n>\n> > > +\t}\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> > > +\tfloat focal_length = 1.0;\n> > > +\texif.setFocalLength(focal_length);\n>\n> \texif.setFocalLength(1.0);\n>\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>\n> As part as the rework of the thumbnail generation, you need to report\n> additional sizes in ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.\n>\n\nThat's not that linear, you know.. I mean, are the newly sizes fixed ?\nin that case it's trivial, but going forward we need to design an\ninterface to query the post-processor derived class to query it\ncapabilities at static metadata intialization time.. maybe a few static\nparameters would do.\n\n> > > +\tret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);\n> > > +\tif (!ret) {\n> > > +\t\t/* \\todo Handle non-matching aspect ratios */\n> > > +\t\t/* \\todo Handle non-zero orientations */\n>\n> Is this required ? As we handle the orientation by setting it in the\n> Exif tags, we don't need to rotate the thumbnail, is there anything else\n> to handle ?\n>\n> > > +\t\tconst int32_t *data = entry.data.i32;\n> > > +\t\tSize thumbnailSize = { static_cast<uint32_t>(data[0]),\n> > > +\t\t\t\t       static_cast<uint32_t>(data[1]) };\n> > > +\n> > > +\t\tstd::vector<unsigned char> thumbnail;\n> > > +\t\tif (thumbnailSize != Size(0, 0)) {\n> > > +\t\t\tconfigureThumbnailer(thumbnailSize, thumbnailer_.pixelFormat());\n> > > +\t\t\tgenerateThumbnail(source, &thumbnail);\n> > > +\t\t}\n> > >  \t\texif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n>\n> You shouldn't set a thumbnail at all if the size is { 0, 0 }.\n>\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> > > +\t\texif.setGPSMethod(method);\n> > > +\t\tresultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,\n> > > +\t\t\t\t\t entry.data.u8, 32);\n> > > +\t}\n> > > +\n> > >  \tif (exif.generate() != 0)\n> > >  \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > >\n> > > @@ -133,13 +205,15 @@ 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> > > +\tresultMetadata->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> > > +\tret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);\n> > > +\tconst uint32_t jpeg_quality = ret ? 95 : *entry.data.u8;\n> > > +\tresultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n>\n> If we start acting on ANDROID_JPEG_QUALITY, we should really take it\n> into account and configure the JPEG encoder accordingly. This can be\n> done on top.\n>\n> > >\n> > > -\tconst uint32_t jpeg_orientation = 0;\n> > > -\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n> > > +\tret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);\n> > > +\tconst uint32_t jpeg_orientation = ret ? 0 : *entry.data.i32;\n> > > +\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\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 5afa831c..d07c9fe5 100644\n> > > --- a/src/android/jpeg/post_processor_jpeg.h\n> > > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > > @@ -26,11 +26,14 @@ public:\n> > >  \t\t      const libcamera::StreamConfiguration &outcfg) override;\n> > >  \tint process(const libcamera::FrameBuffer &source,\n> > >  \t\t    libcamera::Span<uint8_t> destination,\n> > > -\t\t    CameraMetadata *metadata) override;\n> > > +\t\t    const CameraMetadata &requestMetadata,\n> > > +\t\t    CameraMetadata *resultMetadata) override;\n>\n> I'd split this change and the similar one below to a separate patch, it\n> will make this patch easier to review.\n>\n> > >\n> > >  private:\n> > >  \tvoid generateThumbnail(const libcamera::FrameBuffer &source,\n> > >  \t\t\t       std::vector<unsigned char> *thumbnail);\n> > > +\tint configureThumbnailer(const libcamera::Size &size,\n> > > +\t\t\t\t const libcamera::PixelFormat &pixelFormat);\n> > >\n> > >  \tCameraDevice *const cameraDevice_;\n> > >  \tstd::unique_ptr<Encoder> encoder_;\n> > > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> > > index 98f11833..f393db47 100644\n> > > --- a/src/android/jpeg/thumbnailer.h\n> > > +++ b/src/android/jpeg/thumbnailer.h\n> > > @@ -22,6 +22,7 @@ public:\n> > >  \tvoid createThumbnail(const libcamera::FrameBuffer &source,\n> > >  \t\t\t     std::vector<unsigned char> *dest);\n> > >  \tconst libcamera::Size &size() const { return targetSize_; }\n> > > +\tconst libcamera::PixelFormat &pixelFormat() const { return pixelFormat_; }\n> > >\n> > >  private:\n> > >  \tlibcamera::Size computeThumbnailSize() const;\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<uint8_t> 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__ */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 8E146BD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 18 Jan 2021 09:35:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF6EE68115;\n\tMon, 18 Jan 2021 10:35:23 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB85660314\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Jan 2021 10:35:22 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 0A0E62001C;\n\tMon, 18 Jan 2021 09:35:21 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 18 Jan 2021 10:35:41 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210118093541.fqgqk2g4qmpsifb2@uno.localdomain>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-7-paul.elder@ideasonboard.com>\n\t<20210115154812.hbpc5lfgciw6xohq@uno.localdomain>\n\t<YAHvMHl0TAbWUpnZ@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<YAHvMHl0TAbWUpnZ@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 6/6] android: Set result metadata and\n\tEXIF fields based on request metadata","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14627,"web_url":"https://patchwork.libcamera.org/comment/14627/","msgid":"<20210120102905.GC1943@pyrite.rasen.tech>","date":"2021-01-20T10:29:05","subject":"Re: [libcamera-devel] [PATCH 6/6] android: Set result metadata and\n\tEXIF fields based on request metadata","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent and Jacopo,\n\nThank you for the review.\n\nOn Fri, Jan 15, 2021 at 09:38:24PM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Fri, Jan 15, 2021 at 04:48:12PM +0100, Jacopo Mondi wrote:\n> > On Thu, Jan 14, 2021 at 07:40:35PM +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> Can you split this in multiple patches ? Review is difficult. You can\n> have one patch that sets all the result metadata and Exif tags that do\n> not interact with any other component, and one patch per change for all\n> other changes. For instance ANDROID_JPEG_THUMBNAIL_SIZE requires\n> configuring the thumbnailer accordingly, and should be in a patch of its\n> own.\n\nOkay :/\n\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> > \\o/\n> >\n> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > ---\n> > >  src/android/camera_device.cpp            |  27 +++++-\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 | 116 +++++++++++++++++++----\n> > >  src/android/jpeg/post_processor_jpeg.h   |   5 +-\n> > >  src/android/jpeg/thumbnailer.h           |   1 +\n> > >  src/android/post_processor.h             |   3 +-\n> > >  8 files changed, 136 insertions(+), 32 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index ed47c7cd..8d697080 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> > >  {\n> > >  \tbuffers_ = new camera3_stream_buffer_t[numBuffers];\n> > >\n> > > @@ -1700,9 +1701,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> \n> Can it happen ? If so it should be handled as part of this series or\n> immediately on top, not left for later.\n\nAccording to the android docs, yes. In that case the last request\nsettings should be applied, so I think some small plumbing is necessary\nto get that working.\n\nI guess I'll do it for v2 then. idk if there are any tests to confirm\nit though.\n\n> > > +\tCameraMetadata requestMetadata(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     requestMetadata);\n> >\n> > As you use settings, I would name it settings here as well\n> >\n> > >\n> > >  \tLOG(HAL, Debug) << \"Queueing Request to libcamera with \"\n> > >  \t\t\t<< descriptor->numBuffers_ << \" HAL streams\";\n> > > @@ -1815,6 +1819,9 @@ void CameraDevice::requestComplete(Request *request)\n> > >  \t\tCameraStream *cameraStream =\n> > >  \t\t\tstatic_cast<CameraStream *>(descriptor->buffers_[i].stream->priv);\n> > >\n> > > +\t\tfloat focal_length = 1.0;\n> > > +\t\tresultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);\n> > > +\n> >\n> > Ups, result metadata are procesed in getResultMetadata() and if you\n> > add an entry you need to expand the metadata pack size. Also.. see\n> > below..\n> >\n> > >  \t\tif (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)\n> > >  \t\t\tcontinue;\n> > >\n> > > @@ -1837,6 +1844,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> >\n> > I would move:\n> > 1) Adding JPEG tags to EXIF\n> > 2) Filling in the result metadata\n> >\n> > to 2 different patches\n> >\n> > However, I'm a bit skeptic about this change.\n> > You pass to the JPEG encode the settings the application has\n> > requested. The actual process should go through the pipeline handler,\n> > so that you should be able to find all the required information in\n> > libcamera::Request::metadata()\n> >\n> > This requires several parts:\n> > 1) Define a libcamera (draft) control to be able to translate the\n> > Android metadata to a control the libcamera pipeline handler can\n> > consume\n> > 2) Set the libcamera control in the Request's metadata\n> > 3) Parse the control value in the completion handler here.\n> >\n> > You can see how (parts of) this process is handled for draft::PipelineDepth.\n> >\n> > However, it's not trivial to make all the pieces fit together. I have\n> > a series of patches I still have to send out that implement exactly\n> > this for SCALER_CROP_REGION (except for the fact no ad-hoc property\n> > needs to be defined, as we already have\n> > libcamera::controls::ScalerCrop)\n> >\n> > I'm not sure if we should either rush and take this big shortcut here\n> > to provide the lens info to the post-processor, or rather hard-code it\n> > there with a big \\todo, or maybe wait until we close the loop.. I\n> > would go for the second option, but I would like to know Laurent's\n> > opinion here.\n> \n> There are Android metadata that are meant only for the HAL, such as all\n> the JPEG-related information as we don't handle JPEG encoding in\n> libcamera. Passing the request settings to the post-processor does make\n> sense.\n> \n> Looking at the code below, the only metadata entry that needs to be\n> handled by libcamera is the lens aperture. Given that we support a\n> single aperture at the moment (we report a single entry in\n> ANDROID_LENS_INFO_AVAILABLE_APERTURES), hardcoding ANDROID_LENS_APERTURE\n> makes sense, we don't need to define libcamera controls yet. However,\n> setting ANDROID_LENS_APERTURE doesn't belong to this patch (see below\n> for more comments).\n> \n> Same for ANDROID_LENS_FOCAL_LENGTH, we can hardcode it for now as we\n> have a fixed focus lens, but it should go to getResultMetadata() as\n> you've mentioned.\n\nI'll leave a big todo that some settings need to be translated to\nlibcamera controls.\n\n> > >  \t\t\t\t\t\tresultMetadata.get());\n> > >  \t\tif (ret) {\n> > >  \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > > @@ -1933,14 +1941,23 @@ 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: 41 entries, 187 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_ORIENTATION (int32_t) = 3 entries, 9 bytes.\n> >\n> > Can you move the additional JPEG sizes to the end ?\n> >\n> >         = 10 entries, 92 bytes\n> >\n> > However I count:\n> >         41 - 33 = +8\n> >         We have 10 JPEG metadata so I assume the additional one is for\n> >         LENS_FOCAL_LENGTH you add here above\n> >\n> >         187 - 75 = 112\n> >         I count 92 bytes for JPEG + 4 for LENS_FOCAL_LENGHT\n> >\n> >         Where do the additional 16 bytes come from ?\n\nThat's what I counted too, so I'm not sure why CameraMetadata complained\nwhen I had that number. I'll double-check.\n\n> > > +\t * ANDROID_JPEG_THUMBNAIL_SIZE (int32 x 2) = 8 bytes\n> > > +\t * ANDROID_JPEG_THUMBNAIL_QUALITY (int32 x 2) = 8 bytes\n> > > +\t * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes\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_LENS_APERTURE (float) = 4 bytes\n> > > +\t *\n> > > +\t * add coordinates again?\n> >\n> > Not sure I get this todo :)\n\nPersonal note that I forgot to remove :)\n\n> > >  \t */\n> > >  \tstd::unique_ptr<CameraMetadata> resultMetadata =\n> > > -\t\tstd::make_unique<CameraMetadata>(33, 75);\n> > > +\t\tstd::make_unique<CameraMetadata>(41, 187);\n> > >  \tif (!resultMetadata->isValid()) {\n> > >  \t\tLOG(HAL, Error) << \"Failed to allocate static metadata\";\n> > >  \t\treturn nullptr;\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<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;\n> > >  \t\tstd::unique_ptr<CaptureRequest> request_;\n> > > +\t\tCameraMetadata settings_;\n> > >  \t};\n> > >\n> > >  \tstruct Camera3StreamConfiguration {\n> >\n> > The part below should go to a separate patch imho\n\nYeah you're right.\n\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 436a50f8..13ad3777 100644\n> > > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > > @@ -25,6 +25,21 @@ PostProcessorJpeg::PostProcessorJpeg(CameraDevice *const device)\n> > >  {\n> > >  }\n> > >\n> > > +int PostProcessorJpeg::configureThumbnailer(const Size &size,\n> > > +\t\t\t\t\t    const PixelFormat &pixelFormat)\n> > > +{\n> > > +\tthumbnailer_.configure(size, pixelFormat);\n> > > +\tStreamConfiguration thCfg;\n> > > +\tthCfg.size = thumbnailer_.size();\n> > > +\tthCfg.pixelFormat = pixelFormat;\n> > > +\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> > > +\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > >  int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > >  \t\t\t\t const StreamConfiguration &outCfg)\n> > >  {\n> > > @@ -40,16 +55,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,\n> > >\n> > >  \tstreamSize_ = outCfg.size;\n> > >\n> > > -\tthumbnailer_.configure(inCfg.size, inCfg.pixelFormat);\n> > > -\tStreamConfiguration thCfg = inCfg;\n> > > -\tthCfg.size = thumbnailer_.size();\n> > > -\tif (thumbnailEncoder_.configure(thCfg) != 0) {\n> > > -\t\tLOG(JPEG, Error) << \"Failed to configure thumbnail encoder\";\n> > > -\t\treturn -EINVAL;\n> > > -\t}\n> > > +\tint ret = configureThumbnailer(inCfg.size, inCfg.pixelFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > >\n> > >  \tencoder_ = std::make_unique<EncoderLibJpeg>();\n> > > -\n> > >  \treturn encoder_->configure(inCfg);\n> >\n> > Unrelated changes ? Should they go to a third patch ?\n> \n> The introduction of PostProcessorJpeg::configureThumbnailer(), if\n> desired, should be split to a patch of its own, yes.\n\nYeah you're right.\n\n> > I'll review the rest once separated to a new patch\n> >\n> > >  }\n> > >\n> > > @@ -80,17 +90,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,\n> > >\n> > >  int PostProcessorJpeg::process(const FrameBuffer &source,\n> > >  \t\t\t       Span<uint8_t> 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> > If you intend to handle the ANDROID_JPEG_ORIENTATION control (do you\n> > need this?) I think you should combine it with the camera orientation.\n> >\n> > > +\texif.setOrientation(ret ? cameraDevice_->orientation() : *entry.data.i32);\n> > >  \texif.setSize(streamSize_);\n> > >  \t/*\n> > >  \t * We set the frame's EXIF timestamp as the time of encode.\n> > > @@ -99,11 +114,68 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > >  \t */\n> > >  \texif.setTimestamp(std::time(nullptr));\n> > >\n> > > -\tstd::vector<unsigned char> thumbnail;\n> > > -\tgenerateThumbnail(source, &thumbnail);\n> > > -\tif (!thumbnail.empty())\n> > > +\texif.setExposureTime(0);\n> > > +\tret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);\n> > > +\tif (!ret) {\n> > > +\t\texif.setAperture(*entry.data.f);\n> > > +\t\tresultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);\n> \n> This doesn't belong to PostProcessorJpeg::process(), only\n> exif.setAperture() belongs here. ANDROID_LENS_APERTURE should be set in\n> CameraDevice::getResultMetadata().\n> \n> > > +\t}\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> > > +\tfloat focal_length = 1.0;\n> > > +\texif.setFocalLength(focal_length);\n> \n> \texif.setFocalLength(1.0);\n> \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> \n> As part as the rework of the thumbnail generation, you need to report\n> additional sizes in ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.\n> \n> > > +\tret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);\n> > > +\tif (!ret) {\n> > > +\t\t/* \\todo Handle non-matching aspect ratios */\n> > > +\t\t/* \\todo Handle non-zero orientations */\n> \n> Is this required ? As we handle the orientation by setting it in the\n> Exif tags, we don't need to rotate the thumbnail, is there anything else\n> to handle ?\n\nThat's what I saw in the android properties docs.\n\n> > > +\t\tconst int32_t *data = entry.data.i32;\n> > > +\t\tSize thumbnailSize = { static_cast<uint32_t>(data[0]),\n> > > +\t\t\t\t       static_cast<uint32_t>(data[1]) };\n> > > +\n> > > +\t\tstd::vector<unsigned char> thumbnail;\n> > > +\t\tif (thumbnailSize != Size(0, 0)) {\n> > > +\t\t\tconfigureThumbnailer(thumbnailSize, thumbnailer_.pixelFormat());\n> > > +\t\t\tgenerateThumbnail(source, &thumbnail);\n> > > +\t\t}\n> > >  \t\texif.setThumbnail(thumbnail, Exif::Compression::JPEG);\n> \n> You shouldn't set a thumbnail at all if the size is { 0, 0 }.\n> \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> > > +\t\texif.setGPSMethod(method);\n> > > +\t\tresultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,\n> > > +\t\t\t\t\t entry.data.u8, 32);\n> > > +\t}\n> > > +\n> > >  \tif (exif.generate() != 0)\n> > >  \t\tLOG(JPEG, Error) << \"Failed to generate valid EXIF data\";\n> > >\n> > > @@ -133,13 +205,15 @@ 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> > > +\tresultMetadata->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> > > +\tret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);\n> > > +\tconst uint32_t jpeg_quality = ret ? 95 : *entry.data.u8;\n> > > +\tresultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);\n> \n> If we start acting on ANDROID_JPEG_QUALITY, we should really take it\n> into account and configure the JPEG encoder accordingly. This can be\n> done on top.\n\nI noted that and forgot to slap on a \\todo.\n\n> > >\n> > > -\tconst uint32_t jpeg_orientation = 0;\n> > > -\tmetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\n> > > +\tret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);\n> > > +\tconst uint32_t jpeg_orientation = ret ? 0 : *entry.data.i32;\n> > > +\tresultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);\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 5afa831c..d07c9fe5 100644\n> > > --- a/src/android/jpeg/post_processor_jpeg.h\n> > > +++ b/src/android/jpeg/post_processor_jpeg.h\n> > > @@ -26,11 +26,14 @@ public:\n> > >  \t\t      const libcamera::StreamConfiguration &outcfg) override;\n> > >  \tint process(const libcamera::FrameBuffer &source,\n> > >  \t\t    libcamera::Span<uint8_t> destination,\n> > > -\t\t    CameraMetadata *metadata) override;\n> > > +\t\t    const CameraMetadata &requestMetadata,\n> > > +\t\t    CameraMetadata *resultMetadata) override;\n> \n> I'd split this change and the similar one below to a separate patch, it\n> will make this patch easier to review.\n> \n> > >\n> > >  private:\n> > >  \tvoid generateThumbnail(const libcamera::FrameBuffer &source,\n> > >  \t\t\t       std::vector<unsigned char> *thumbnail);\n> > > +\tint configureThumbnailer(const libcamera::Size &size,\n> > > +\t\t\t\t const libcamera::PixelFormat &pixelFormat);\n> > >\n> > >  \tCameraDevice *const cameraDevice_;\n> > >  \tstd::unique_ptr<Encoder> encoder_;\n> > > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h\n> > > index 98f11833..f393db47 100644\n> > > --- a/src/android/jpeg/thumbnailer.h\n> > > +++ b/src/android/jpeg/thumbnailer.h\n> > > @@ -22,6 +22,7 @@ public:\n> > >  \tvoid createThumbnail(const libcamera::FrameBuffer &source,\n> > >  \t\t\t     std::vector<unsigned char> *dest);\n> > >  \tconst libcamera::Size &size() const { return targetSize_; }\n> > > +\tconst libcamera::PixelFormat &pixelFormat() const { return pixelFormat_; }\n> > >\n> > >  private:\n> > >  \tlibcamera::Size computeThumbnailSize() const;\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<uint8_t> 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__ */\n\n\nThanks,\n\nPaul","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 DFDE1C0F2A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 20 Jan 2021 10:29:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AE9868190;\n\tWed, 20 Jan 2021 11:29:15 +0100 (CET)","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 4331F6817D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 20 Jan 2021 11:29:14 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 549C9813;\n\tWed, 20 Jan 2021 11:29:11 +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=\"HAXL6u/V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611138553;\n\tbh=uahIArJoBU68uGQR/SPuw12CndwpsOQnZmQElcf922A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HAXL6u/VC05mn3NRGNzoE4DqP86Xg0XYKDZdo4eIWL/yqIAdhHihqxBs87aVMVQ6F\n\tLtK3420fFIKosaJY9dwQW3ZQgcLZeCz10DrT7ctyLwuKJnuR8/yYczyCxUNgT9A8DX\n\twz7X58Ukgk709LCoGxeyIt6QJGdJ9Neyldk4HUzM=","Date":"Wed, 20 Jan 2021 19:29:05 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210120102905.GC1943@pyrite.rasen.tech>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-7-paul.elder@ideasonboard.com>\n\t<20210115154812.hbpc5lfgciw6xohq@uno.localdomain>\n\t<YAHvMHl0TAbWUpnZ@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<YAHvMHl0TAbWUpnZ@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 6/6] android: Set result metadata and\n\tEXIF fields based on request metadata","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]