[libcamera-devel] [PATCH 6/6] android: Set result metadata and EXIF fields based on request metadata
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jan 15 20:38:24 CET 2021
Hi Paul,
Thank you for the patch.
On Fri, Jan 15, 2021 at 04:48:12PM +0100, Jacopo Mondi wrote:
> On Thu, Jan 14, 2021 at 07:40:35PM +0900, Paul Elder wrote:
> > Set the following android result metadata:
> > - ANDROID_LENS_FOCAL_LENGTH
> > - ANDROID_LENS_APERTURE
> > - ANDROID_JPEG_GPS_TIMESTAMP
> > - ANDROID_JPEG_THUMBNAIL_SIZE
> > - ANDROID_JPEG_THUMBNAIL_QUALITY
> > - ANDROID_JPEG_GPS_COORDINATES
> > - ANDROID_JPEG_GPS_PROCESSING_METHOD
> > - ANDROID_JPEG_SIZE
> > - ANDROID_JPEG_QUALITY
> > - ANDROID_JPEG_ORIENTATION
> >
> > And the following EXIF fields:
> > - GPSDatestamp
> > - GPSTimestamp
> > - GPSLocation
> > - GPSLatitudeRef
> > - GPSLatitude
> > - GPSLongitudeRef
> > - GPSLongitude
> > - GPSAltitudeRef
> > - GPSAltitude
> > - GPSProcessingMethod
> > - FocalLength
> > - ExposureTime
> > - FNumber
> > - ISO
> > - Flash
> > - WhiteBalance
> > - SubsecTime
> > - SubsecTimeOriginal
> > - SubsecTimeDigitized
> >
> > Based on android request metadata.
Can you split this in multiple patches ? Review is difficult. You can
have one patch that sets all the result metadata and Exif tags that do
not interact with any other component, and one patch per change for all
other changes. For instance ANDROID_JPEG_THUMBNAIL_SIZE requires
configuring the thumbnailer accordingly, and should be in a patch of its
own.
> >
> > This allows the following CTS tests to pass:
> > - android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
> > - android.hardware.camera2.cts.StillCaptureTest#testJpegExif
>
> \o/
>
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> > src/android/camera_device.cpp | 27 +++++-
> > src/android/camera_device.h | 5 +-
> > src/android/camera_stream.cpp | 7 +-
> > src/android/camera_stream.h | 4 +-
> > src/android/jpeg/post_processor_jpeg.cpp | 116 +++++++++++++++++++----
> > src/android/jpeg/post_processor_jpeg.h | 5 +-
> > src/android/jpeg/thumbnailer.h | 1 +
> > src/android/post_processor.h | 3 +-
> > 8 files changed, 136 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ed47c7cd..8d697080 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -296,8 +296,9 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> > */
> >
> > CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> > - Camera *camera, unsigned int frameNumber, unsigned int numBuffers)
> > - : frameNumber_(frameNumber), numBuffers_(numBuffers)
> > + Camera *camera, unsigned int frameNumber, unsigned int numBuffers,
> > + CameraMetadata &settings)
> > + : frameNumber_(frameNumber), numBuffers_(numBuffers), settings_(settings)
> > {
> > buffers_ = new camera3_stream_buffer_t[numBuffers];
> >
> > @@ -1700,9 +1701,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > * The descriptor and the associated memory reserved here are freed
> > * at request complete time.
> > */
> > + /* \todo Handle null request settings */
Can it happen ? If so it should be handled as part of this series or
immediately on top, not left for later.
> > + CameraMetadata requestMetadata(camera3Request->settings);
> > Camera3RequestDescriptor *descriptor =
> > new Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number,
> > - camera3Request->num_output_buffers);
> > + camera3Request->num_output_buffers,
> > + requestMetadata);
>
> As you use settings, I would name it settings here as well
>
> >
> > LOG(HAL, Debug) << "Queueing Request to libcamera with "
> > << descriptor->numBuffers_ << " HAL streams";
> > @@ -1815,6 +1819,9 @@ void CameraDevice::requestComplete(Request *request)
> > CameraStream *cameraStream =
> > static_cast<CameraStream *>(descriptor->buffers_[i].stream->priv);
> >
> > + float focal_length = 1.0;
> > + resultMetadata->addEntry(ANDROID_LENS_FOCAL_LENGTH, &focal_length, 1);
> > +
>
> Ups, result metadata are procesed in getResultMetadata() and if you
> add an entry you need to expand the metadata pack size. Also.. see
> below..
>
> > if (cameraStream->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)
> > continue;
> >
> > @@ -1837,6 +1844,7 @@ void CameraDevice::requestComplete(Request *request)
> > }
> >
> > int ret = cameraStream->process(*buffer, &mapped,
> > + descriptor->settings_,
>
> I would move:
> 1) Adding JPEG tags to EXIF
> 2) Filling in the result metadata
>
> to 2 different patches
>
> However, I'm a bit skeptic about this change.
> You pass to the JPEG encode the settings the application has
> requested. The actual process should go through the pipeline handler,
> so that you should be able to find all the required information in
> libcamera::Request::metadata()
>
> This requires several parts:
> 1) Define a libcamera (draft) control to be able to translate the
> Android metadata to a control the libcamera pipeline handler can
> consume
> 2) Set the libcamera control in the Request's metadata
> 3) Parse the control value in the completion handler here.
>
> You can see how (parts of) this process is handled for draft::PipelineDepth.
>
> However, it's not trivial to make all the pieces fit together. I have
> a series of patches I still have to send out that implement exactly
> this for SCALER_CROP_REGION (except for the fact no ad-hoc property
> needs to be defined, as we already have
> libcamera::controls::ScalerCrop)
>
> I'm not sure if we should either rush and take this big shortcut here
> to provide the lens info to the post-processor, or rather hard-code it
> there with a big \todo, or maybe wait until we close the loop.. I
> would go for the second option, but I would like to know Laurent's
> opinion here.
There are Android metadata that are meant only for the HAL, such as all
the JPEG-related information as we don't handle JPEG encoding in
libcamera. Passing the request settings to the post-processor does make
sense.
Looking at the code below, the only metadata entry that needs to be
handled by libcamera is the lens aperture. Given that we support a
single aperture at the moment (we report a single entry in
ANDROID_LENS_INFO_AVAILABLE_APERTURES), hardcoding ANDROID_LENS_APERTURE
makes sense, we don't need to define libcamera controls yet. However,
setting ANDROID_LENS_APERTURE doesn't belong to this patch (see below
for more comments).
Same for ANDROID_LENS_FOCAL_LENGTH, we can hardcode it for now as we
have a fixed focus lens, but it should go to getResultMetadata() as
you've mentioned.
> > resultMetadata.get());
> > if (ret) {
> > status = CAMERA3_BUFFER_STATUS_ERROR;
> > @@ -1933,14 +1941,23 @@ CameraDevice::getResultMetadata(Camera3RequestDescriptor *descriptor,
> >
> > /*
> > * \todo Keep this in sync with the actual number of entries.
> > - * Currently: 33 entries, 75 bytes
> > + * Currently: 41 entries, 187 bytes
> > *
> > * Reserve more space for the JPEG metadata set by the post-processor.
> > * Currently: ANDROID_JPEG_SIZE (int32_t), ANDROID_JPEG_QUALITY (byte),
> > * ANDROID_JPEG_ORIENTATION (int32_t) = 3 entries, 9 bytes.
>
> Can you move the additional JPEG sizes to the end ?
>
> = 10 entries, 92 bytes
>
> However I count:
> 41 - 33 = +8
> We have 10 JPEG metadata so I assume the additional one is for
> LENS_FOCAL_LENGTH you add here above
>
> 187 - 75 = 112
> I count 92 bytes for JPEG + 4 for LENS_FOCAL_LENGHT
>
> Where do the additional 16 bytes come from ?
>
> > + * ANDROID_JPEG_THUMBNAIL_SIZE (int32 x 2) = 8 bytes
> > + * ANDROID_JPEG_THUMBNAIL_QUALITY (int32 x 2) = 8 bytes
> > + * ANDROID_LENS_FOCAL_LENGTH (float) = 4 bytes
> > + * ANDROID_JPEG_GPS_COORDINATES (double x 3) = 24 bytes
> > + * ANDROID_JPEG_GPS_PROCESSING_METHOD (byte x 32) = 32 bytes
> > + * ANDROID_JPEG_GPS_TIMESTAMP (int64) = 8 bytes
> > + * ANDROID_LENS_APERTURE (float) = 4 bytes
> > + *
> > + * add coordinates again?
>
> Not sure I get this todo :)
>
> > */
> > std::unique_ptr<CameraMetadata> resultMetadata =
> > - std::make_unique<CameraMetadata>(33, 75);
> > + std::make_unique<CameraMetadata>(41, 187);
> > if (!resultMetadata->isValid()) {
> > LOG(HAL, Error) << "Failed to allocate static metadata";
> > return nullptr;
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index a285d0a1..111a7d8f 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -24,6 +24,7 @@
> > #include "libcamera/internal/log.h"
> > #include "libcamera/internal/message.h"
> >
> > +#include "camera_metadata.h"
> > #include "camera_stream.h"
> > #include "camera_worker.h"
> > #include "jpeg/encoder.h"
> > @@ -78,7 +79,8 @@ private:
> > struct Camera3RequestDescriptor {
> > Camera3RequestDescriptor(libcamera::Camera *camera,
> > unsigned int frameNumber,
> > - unsigned int numBuffers);
> > + unsigned int numBuffers,
> > + CameraMetadata &settings);
> > ~Camera3RequestDescriptor();
> >
> > uint32_t frameNumber_;
> > @@ -86,6 +88,7 @@ private:
> > camera3_stream_buffer_t *buffers_;
> > std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> > std::unique_ptr<CaptureRequest> request_;
> > + CameraMetadata settings_;
> > };
> >
> > struct Camera3StreamConfiguration {
>
> The part below should go to a separate patch imho
>
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index dba351a4..4c8020e5 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -96,12 +96,15 @@ int CameraStream::configure()
> > }
> >
> > int CameraStream::process(const libcamera::FrameBuffer &source,
> > - MappedCamera3Buffer *dest, CameraMetadata *metadata)
> > + MappedCamera3Buffer *dest,
> > + const CameraMetadata &requestMetadata,
> > + CameraMetadata *resultMetadata)
> > {
> > if (!postProcessor_)
> > return 0;
> >
> > - return postProcessor_->process(source, dest->maps()[0], metadata);
> > + return postProcessor_->process(source, dest->maps()[0],
> > + requestMetadata, resultMetadata);
> > }
> >
> > FrameBuffer *CameraStream::getBuffer()
> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> > index cc9d5470..298ffbf6 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -119,7 +119,9 @@ public:
> >
> > int configure();
> > int process(const libcamera::FrameBuffer &source,
> > - MappedCamera3Buffer *dest, CameraMetadata *metadata);
> > + MappedCamera3Buffer *dest,
> > + const CameraMetadata &requestMetadata,
> > + CameraMetadata *resultMetadata);
> > libcamera::FrameBuffer *getBuffer();
> > void putBuffer(libcamera::FrameBuffer *buffer);
> >
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > index 436a50f8..13ad3777 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -25,6 +25,21 @@ PostProcessorJpeg::PostProcessorJpeg(CameraDevice *const device)
> > {
> > }
> >
> > +int PostProcessorJpeg::configureThumbnailer(const Size &size,
> > + const PixelFormat &pixelFormat)
> > +{
> > + thumbnailer_.configure(size, pixelFormat);
> > + StreamConfiguration thCfg;
> > + thCfg.size = thumbnailer_.size();
> > + thCfg.pixelFormat = pixelFormat;
> > + if (thumbnailEncoder_.configure(thCfg) != 0) {
> > + LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> > const StreamConfiguration &outCfg)
> > {
> > @@ -40,16 +55,11 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
> >
> > streamSize_ = outCfg.size;
> >
> > - thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> > - StreamConfiguration thCfg = inCfg;
> > - thCfg.size = thumbnailer_.size();
> > - if (thumbnailEncoder_.configure(thCfg) != 0) {
> > - LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> > - return -EINVAL;
> > - }
> > + int ret = configureThumbnailer(inCfg.size, inCfg.pixelFormat);
> > + if (ret)
> > + return ret;
> >
> > encoder_ = std::make_unique<EncoderLibJpeg>();
> > -
> > return encoder_->configure(inCfg);
>
> Unrelated changes ? Should they go to a third patch ?
The introduction of PostProcessorJpeg::configureThumbnailer(), if
desired, should be split to a patch of its own, yes.
> I'll review the rest once separated to a new patch
>
> > }
> >
> > @@ -80,17 +90,22 @@ void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> >
> > int PostProcessorJpeg::process(const FrameBuffer &source,
> > Span<uint8_t> destination,
> > - CameraMetadata *metadata)
> > + const CameraMetadata &requestMetadata,
> > + CameraMetadata *resultMetadata)
> > {
> > if (!encoder_)
> > return 0;
> >
> > + camera_metadata_ro_entry_t entry;
> > + int ret;
> > +
> > /* Set EXIF metadata for various tags. */
> > Exif exif;
> > - /* \todo Set Make and Model from external vendor tags. */
> > - exif.setMake("libcamera");
> > - exif.setModel("cameraModel");
> > - exif.setOrientation(cameraDevice_->orientation());
> > + exif.setMake(cameraDevice_->cameraMake());
> > + exif.setModel(cameraDevice_->cameraModel());
> > +
> > + ret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);
>
> If you intend to handle the ANDROID_JPEG_ORIENTATION control (do you
> need this?) I think you should combine it with the camera orientation.
>
> > + exif.setOrientation(ret ? cameraDevice_->orientation() : *entry.data.i32);
> > exif.setSize(streamSize_);
> > /*
> > * We set the frame's EXIF timestamp as the time of encode.
> > @@ -99,11 +114,68 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > */
> > exif.setTimestamp(std::time(nullptr));
> >
> > - std::vector<unsigned char> thumbnail;
> > - generateThumbnail(source, &thumbnail);
> > - if (!thumbnail.empty())
> > + exif.setExposureTime(0);
> > + ret = requestMetadata.getEntry(ANDROID_LENS_APERTURE, &entry);
> > + if (!ret) {
> > + exif.setAperture(*entry.data.f);
> > + resultMetadata->addEntry(ANDROID_LENS_APERTURE, entry.data.f, 1);
This doesn't belong to PostProcessorJpeg::process(), only
exif.setAperture() belongs here. ANDROID_LENS_APERTURE should be set in
CameraDevice::getResultMetadata().
> > + }
> > + exif.setISO(100);
> > + exif.setFlash(Exif::Flash::FlashNotPresent);
> > + exif.setWhiteBalance(Exif::WhiteBalance::Auto);
> > +
> > + exif.setSubsecTime(0);
> > + exif.setSubsecTimeOriginal(0);
> > + exif.setSubsecTimeDigitized(0);
> > +
> > + float focal_length = 1.0;
> > + exif.setFocalLength(focal_length);
exif.setFocalLength(1.0);
> > +
> > + ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_TIMESTAMP, &entry);
> > + if (!ret) {
> > + exif.setGPSDateTimestamp(*entry.data.i64);
> > + resultMetadata->addEntry(ANDROID_JPEG_GPS_TIMESTAMP,
> > + entry.data.i64, 1);
> > + }
> > +
As part as the rework of the thumbnail generation, you need to report
additional sizes in ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.
> > + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);
> > + if (!ret) {
> > + /* \todo Handle non-matching aspect ratios */
> > + /* \todo Handle non-zero orientations */
Is this required ? As we handle the orientation by setting it in the
Exif tags, we don't need to rotate the thumbnail, is there anything else
to handle ?
> > + const int32_t *data = entry.data.i32;
> > + Size thumbnailSize = { static_cast<uint32_t>(data[0]),
> > + static_cast<uint32_t>(data[1]) };
> > +
> > + std::vector<unsigned char> thumbnail;
> > + if (thumbnailSize != Size(0, 0)) {
> > + configureThumbnailer(thumbnailSize, thumbnailer_.pixelFormat());
> > + generateThumbnail(source, &thumbnail);
> > + }
> > exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
You shouldn't set a thumbnail at all if the size is { 0, 0 }.
> >
> > + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
> > +
> > + /* \todo Use this quality as a parameter to the encoder */
> > + ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);
> > + if (!ret)
> > + resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, entry.data.u8, 1);
> > + }
> > +
> > + ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_COORDINATES, &entry);
> > + if (!ret) {
> > + exif.setGPSLocation(entry.data.d);
> > + resultMetadata->addEntry(ANDROID_JPEG_GPS_COORDINATES,
> > + entry.data.d, 3);
> > + }
> > +
> > + ret = requestMetadata.getEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD, &entry);
> > + if (!ret) {
> > + std::string method(entry.data.u8, entry.data.u8 + 32);
> > + exif.setGPSMethod(method);
> > + resultMetadata->addEntry(ANDROID_JPEG_GPS_PROCESSING_METHOD,
> > + entry.data.u8, 32);
> > + }
> > +
> > if (exif.generate() != 0)
> > LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> >
> > @@ -133,13 +205,15 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > blob->jpeg_size = jpeg_size;
> >
> > /* Update the JPEG result Metadata. */
> > - metadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> > + resultMetadata->addEntry(ANDROID_JPEG_SIZE, &jpeg_size, 1);
> >
> > - const uint32_t jpeg_quality = 95;
> > - metadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
> > + ret = requestMetadata.getEntry(ANDROID_JPEG_QUALITY, &entry);
> > + const uint32_t jpeg_quality = ret ? 95 : *entry.data.u8;
> > + resultMetadata->addEntry(ANDROID_JPEG_QUALITY, &jpeg_quality, 1);
If we start acting on ANDROID_JPEG_QUALITY, we should really take it
into account and configure the JPEG encoder accordingly. This can be
done on top.
> >
> > - const uint32_t jpeg_orientation = 0;
> > - metadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> > + ret = requestMetadata.getEntry(ANDROID_JPEG_ORIENTATION, &entry);
> > + const uint32_t jpeg_orientation = ret ? 0 : *entry.data.i32;
> > + resultMetadata->addEntry(ANDROID_JPEG_ORIENTATION, &jpeg_orientation, 1);
> >
> > return 0;
> > }
> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > index 5afa831c..d07c9fe5 100644
> > --- a/src/android/jpeg/post_processor_jpeg.h
> > +++ b/src/android/jpeg/post_processor_jpeg.h
> > @@ -26,11 +26,14 @@ public:
> > const libcamera::StreamConfiguration &outcfg) override;
> > int process(const libcamera::FrameBuffer &source,
> > libcamera::Span<uint8_t> destination,
> > - CameraMetadata *metadata) override;
> > + const CameraMetadata &requestMetadata,
> > + CameraMetadata *resultMetadata) override;
I'd split this change and the similar one below to a separate patch, it
will make this patch easier to review.
> >
> > private:
> > void generateThumbnail(const libcamera::FrameBuffer &source,
> > std::vector<unsigned char> *thumbnail);
> > + int configureThumbnailer(const libcamera::Size &size,
> > + const libcamera::PixelFormat &pixelFormat);
> >
> > CameraDevice *const cameraDevice_;
> > std::unique_ptr<Encoder> encoder_;
> > diff --git a/src/android/jpeg/thumbnailer.h b/src/android/jpeg/thumbnailer.h
> > index 98f11833..f393db47 100644
> > --- a/src/android/jpeg/thumbnailer.h
> > +++ b/src/android/jpeg/thumbnailer.h
> > @@ -22,6 +22,7 @@ public:
> > void createThumbnail(const libcamera::FrameBuffer &source,
> > std::vector<unsigned char> *dest);
> > const libcamera::Size &size() const { return targetSize_; }
> > + const libcamera::PixelFormat &pixelFormat() const { return pixelFormat_; }
> >
> > private:
> > libcamera::Size computeThumbnailSize() const;
> > diff --git a/src/android/post_processor.h b/src/android/post_processor.h
> > index e0f91880..bda93bb4 100644
> > --- a/src/android/post_processor.h
> > +++ b/src/android/post_processor.h
> > @@ -22,7 +22,8 @@ public:
> > const libcamera::StreamConfiguration &outCfg) = 0;
> > virtual int process(const libcamera::FrameBuffer &source,
> > libcamera::Span<uint8_t> destination,
> > - CameraMetadata *metadata) = 0;
> > + const CameraMetadata &requestMetadata,
> > + CameraMetadata *resultMetadata) = 0;
> > };
> >
> > #endif /* __ANDROID_POST_PROCESSOR_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list