[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:03:21 CEST 2020
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.
> >
> > 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.
> 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 ?
> ¯\_(ツ)_/¯
>
> >> + }
> >> +
> >> + 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