[libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an initial set of EXIF metadata tags
Jacopo Mondi
jacopo at jmondi.org
Tue Sep 8 14:43:46 CEST 2020
Hi Laurent, Umang,
On Tue, Sep 08, 2020 at 03:03:21PM +0300, Laurent Pinchart wrote:
> Hi Umang,
>
> 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' ?
> > > 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
> > 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.
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.
Thanks
j
> > ¯\_(ツ)_/¯
> >
> > >> + }
> > >> +
> > >> + 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