[libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an initial set of EXIF metadata tags
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 8 14:46:14 CEST 2020
Hi Jacopo,
On Tue, Sep 08, 2020 at 02:43:46PM +0200, Jacopo Mondi wrote:
> On Tue, Sep 08, 2020 at 03:03:21PM +0300, Laurent Pinchart wrote:
> > On Tue, Sep 08, 2020 at 03:42:39PM +0530, Umang Jain wrote:
> >> On 9/4/20 9:04 PM, Laurent Pinchart wrote:
> >>> On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote:
> >>>> Create a Exif object with various metadata tags set, just before
> >>>> the encoder starts to encode the frame. The object is passed
> >>>> directly as libcamera::Span<> to make sure EXIF tags can be set
> >>>> in a single place i.e. in CameraDevice and the encoder only has
> >>>> the job to write the data in the final output.
> >>>>
> >>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>> Signed-off-by: Umang Jain <email at uajain.com>
> >>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >>>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>>> ---
> >>>> src/android/camera_device.cpp | 19 ++++++++++-
> >>>> src/android/jpeg/encoder.h | 3 +-
> >>>> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++-
> >>>> src/android/jpeg/encoder_libjpeg.h | 3 +-
> >>>> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++
> >>>> src/android/jpeg/exif.h | 8 +++++
> >>>> 6 files changed, 88 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >>>> index de6f86f..0328ac6 100644
> >>>> --- a/src/android/camera_device.cpp
> >>>> +++ b/src/android/camera_device.cpp
> >>>> @@ -24,6 +24,7 @@
> >>>> #include "system/graphics.h"
> >>>>
> >>>> #include "jpeg/encoder_libjpeg.h"
> >>>> +#include "jpeg/exif.h"
> >>>>
> >>>> using namespace libcamera;
> >>>>
> >>>> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request)
> >>>> continue;
> >>>> }
> >>>>
> >>>> - int jpeg_size = encoder->encode(buffer, mapped.maps()[0]);
> >>>> + /* 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(orientation_);
> >>>> + exif.setSize(cameraStream->size);
> >>>> + /*
> >>>> + * We set the frame's EXIF timestamp as the time of encode.
> >>>> + * Since the precision we need for EXIF timestamp is only one
> >>>> + * second, it is good enough.
> >>>> + */
> >>>> + exif.setTimestamp(std::time(nullptr));
> >>>> + if (exif.generate() != 0)
> >>>> + LOG(HAL, Error) << "Failed to get valid EXIF data";
> >>>
> >>> s/get/generate/ ?
> >>>
> >>>> +
> >>>> + int jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif.data());
> >>>> if (jpeg_size < 0) {
> >>>> LOG(HAL, Error) << "Failed to encode stream image";
> >>>> status = CAMERA3_BUFFER_STATUS_ERROR;
> >>>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> >>>> index f9eb88e..cf26d67 100644
> >>>> --- a/src/android/jpeg/encoder.h
> >>>> +++ b/src/android/jpeg/encoder.h
> >>>> @@ -18,7 +18,8 @@ public:
> >>>>
> >>>> virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
> >>>> virtual int encode(const libcamera::FrameBuffer *source,
> >>>> - const libcamera::Span<uint8_t> &destination) = 0;
> >>>> + const libcamera::Span<uint8_t> &destination,
> >>>> + const libcamera::Span<const uint8_t> &exifData) = 0;
> >>>> };
> >>>>
> >>>> #endif /* __ANDROID_JPEG_ENCODER_H__ */
> >>>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> >>>> index 977538c..510613c 100644
> >>>> --- a/src/android/jpeg/encoder_libjpeg.cpp
> >>>> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> >>>> @@ -180,7 +180,8 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
> >>>> }
> >>>>
> >>>> int EncoderLibJpeg::encode(const FrameBuffer *source,
> >>>> - const libcamera::Span<uint8_t> &dest)
> >>>> + const libcamera::Span<uint8_t> &dest,
> >>>> + const libcamera::Span<const uint8_t> &exifData)
> >>>> {
> >>>> MappedFrameBuffer frame(source, PROT_READ);
> >>>> if (!frame.isValid()) {
> >>>> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,
> >>>>
> >>>> jpeg_start_compress(&compress_, TRUE);
> >>>>
> >>>> + if (exifData.size())
> >>>> + /* Store Exif data in the JPEG_APP1 data block. */
> >>>> + jpeg_write_marker(&compress_, JPEG_APP0 + 1,
> >>>> + static_cast<const JOCTET *>(exifData.data()),
> >>>> + exifData.size());
> >>>> +
> >>>> LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width
> >>>> << "x" << compress_.image_height;
> >>>>
> >>>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> >>>> index aed081a..1e8df05 100644
> >>>> --- a/src/android/jpeg/encoder_libjpeg.h
> >>>> +++ b/src/android/jpeg/encoder_libjpeg.h
> >>>> @@ -22,7 +22,8 @@ public:
> >>>>
> >>>> int configure(const libcamera::StreamConfiguration &cfg) override;
> >>>> int encode(const libcamera::FrameBuffer *source,
> >>>> - const libcamera::Span<uint8_t> &destination) override;
> >>>> + const libcamera::Span<uint8_t> &destination,
> >>>> + const libcamera::Span<const uint8_t> &exifData) override;
> >>>>
> >>>> private:
> >>>> void compressRGB(const libcamera::MappedBuffer *frame);
> >>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> >>>> index 41ddf48..36922ef 100644
> >>>> --- a/src/android/jpeg/exif.cpp
> >>>> +++ b/src/android/jpeg/exif.cpp
> >>>> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str
> >>>> exif_entry_unref(entry);
> >>>> }
> >>>>
> >>>> +void Exif::setMake(const std::string &make)
> >>>> +{
> >>>> + setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);
> >>>> +}
> >>>> +
> >>>> +void Exif::setModel(const std::string &model)
> >>>> +{
> >>>> + setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);
> >>>> +}
> >>>> +
> >>>> +void Exif::setSize(const Size &size)
> >>>> +{
> >>>> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
> >>>> + setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
> >>>> +}
> >>>> +
> >>>> +void Exif::setTimestamp(time_t timestamp)
> >>>> +{
> >>>> + char str[20];
> >>>> + std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S",
> >>>> + std::localtime(×tamp));
> >>>> + std::string ts(str);
> >>>> +
> >>>> + setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);
> >>>> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);
> >>>> + setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);
> >>>> +}
> >>>> +
> >>>> +void Exif::setOrientation(int orientation)
> >>>> +{
> >>>> + int value;
> >>>> + switch (orientation) {
> >>>> + case 0:
> >>>> + default:
> >>>> + value = 1;
> >>>> + break;
> >>>> + case 90:
> >>>> + value = 6;
> >>>> + break;
> >>>> + case 180:
> >>>> + value = 3;
> >>>> + break;
> >>>> + case 270:
> >>>> + value = 8;
> >>>> + break;
> >>>
> >>> Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is
> >>> the visual right-hand side of the image, and the 0th column is the
> >>> visual top", and 8 means "The 0th row is the visual left-hand side of
> >>> the image, and the 0th column is the visual bottom".
> >>>
> >>> Looking at the rotation property definition, the 90° rotation examples
> >>> shows
> >>>
> >>> +-------------------------------------+
> >>> | _ _ |
> >>> | \ / |
> >>> | | | |
> >>> | | | |
> >>> | | > |
> >>> | < | |
> >>> | | | |
> >>> | . |
> >>> | V |
> >>> +-------------------------------------+
> >>>
> >>> where the 0th row is the left side and the 0th column the bottom side.
>
> Isn't the 0-th row (the first row of the image) on the 'visual
> right-hand side' ?
Here's my interpretation:
Looking at the fish in front of you (forgetting the camera), the visual
right-hand side is the head, and the visual left-hand side is the tail.
The 0th row of the image (now we're talking about the buffer, as
numbering rows implies we're counting pixels, so it's the buffer, not
the scene anymore). is the tail, thus the visual left-hand side of the
image.
> >>> I can fix those two small issues when applying, but I'd appreciate if
> >>> someone could double-check the rotation.
> >>
> >> Does the rotation property has to do anything with the fact that, camera
> >> sensor(webcam use-case) is intentionally mounted upside down to avoid
> >> rotation correction?
> >
> > The rotation reported by libcamera exposes the optical rotation of the
> > camera module, combining both the camera sensor and the lens. It is
> > indeed common to mount the camera sensor upside-down to compensate for
> > the lens inverting the image. In that case the reported rotation is 0°,
> > as the camera sensor rotation and the lens inversion compensate each
> > other.
>
> The 'rotation' property definition takes into account the lens
> inversion, so you shouldn't have to care about it
I'm not sure if this means you agree or disagree with my explanation :-)
> >> A widely used example on the internet is a big 'F'
> >> image denoting the value according to it's orientation [1], which is the
> >> same as per the values used in the patch. On the other hand, I can also
> >> find the code which satisfies Laurent's review and the EXIF
> >> 'orientation' specification [2]
> >>
> >> [1]:
> >> https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376
> >> [2]:
> >> https://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation
> >
> > The 0° and 180° cases are fairly straightforward, they map to the EXIF
> > values 1 and 3 respectively, there's usually no disagreement on that.
> >
> > The 90° and 270° cases are much more error prone as it's easy to get the
> > rotation direction wrong. The mapping between the 'F' orientation and
> > the EXIF values shown in [1] seems right to me.
> >
> > Android defines the rotation as the clockwise angle by which the image
> > needs to be rotated to appear in the correct orientation on the device
> > screen.
> >
> > For EXIF value 6,
> >
> > 88
> > 88 88
> > 8888888888
> >
> > the image needs to be rotated by 90° clockwise, while for EXIF value 8,
> >
> > 8888888888
> > 88 88
> > 88
> >
> > it needs to be rotated by 270°. The code in [2] thus appears correct.
> > What however appears to be incorrect is our claim that the libcamera and
> > Android numerical values are the same.
> >
> > Jacopo, any opinion on that ?
>
> Our definition differs from the Android one, as the rotation value we
> report is the rotation correction degrees in counter-clockwise direction.
> Android defines that in the clockwise direction.
So there's a bug in our HAL implementation when reporting the rotation ?
> If I got the above discussion right and looking at the 'F' examples in
> [1], we should report:
> 90 degrees = 8
> 270 degrees = 6
>
> However, this interpreation and the 'F' examples, don't match the
> textual description Laurent reported above:
>
> -------------------------------------------------------------------------------
> Shouldn't the the 90 and 270 values be swapped ? 6 means "The 0th row is
> the visual right-hand side of the image, and the 0th column is the
> visual top", and 8 means "The 0th row is the visual left-hand side of
> the image, and the 0th column is the visual bottom".
> ------------------------------------------------------------------------------
>
> For EXIF value 6,
>
> 88
> 88 88
> 8888888888
>
> the image needs to be rotated by 90° clockwise, while for EXIF value 8,
>
> 8888888888
> 88 88
> 88
> ------------------------------------------------------------------------------
>
> The two do not match, and I wonder what's the authoritative source for
> the definition of the semantic associated with the '6' and '8'
> values.
The EXIF specification is the authoritative source, and it's defined as
I've quoted.
> >> ¯\_(ツ)_/¯
> >>
> >>>> + }
> >>>> +
> >>>> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> >>>> +}
> >>>> +
> >>>> [[nodiscard]] int Exif::generate()
> >>>> {
> >>>> if (exifData_) {
> >>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> >>>> index 8dfc324..622de4c 100644
> >>>> --- a/src/android/jpeg/exif.h
> >>>> +++ b/src/android/jpeg/exif.h
> >>>> @@ -12,6 +12,7 @@
> >>>>
> >>>> #include <libexif/exif-data.h>
> >>>>
> >>>> +#include <libcamera/geometry.h>
> >>>> #include <libcamera/span.h>
> >>>>
> >>>> class Exif
> >>>> @@ -20,6 +21,13 @@ public:
> >>>> Exif();
> >>>> ~Exif();
> >>>>
> >>>> + void setMake(const std::string &make);
> >>>> + void setModel(const std::string &model);
> >>>> +
> >>>> + void setOrientation(int orientation);
> >>>> + void setSize(const libcamera::Size &size);
> >>>> + void setTimestamp(time_t timestamp);
> >>>> +
> >>>> libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> >>>> [[nodiscard]] int generate();
> >>>>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list