[libcamera-devel] [PATCH v5 2/2] android: jpeg: Support an initial set of EXIF metadata tags
Umang Jain
email at uajain.com
Tue Sep 8 07:45:09 CEST 2020
Hi Laurent,
On 9/4/20 6:14 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Fri, Sep 04, 2020 at 11:35:08AM +0530, Umang Jain wrote:
>> On 9/4/20 4:10 AM, Laurent Pinchart wrote:
>>> On Thu, Sep 03, 2020 at 07:38:07PM +0100, Kieran Bingham wrote:
>>>> On 03/09/2020 17:32, 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>
>>>>> ---
>>>>> src/android/camera_device.cpp | 18 ++++++++++-
>>>>> 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 | 48 +++++++++++++++++++++++++++-
>>>>> src/android/jpeg/exif.h | 8 +++++
>>>>> 6 files changed, 84 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>>>>> index de6f86f..54ca9c6 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,22 @@ 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));
>>>>> + exif.generate();
>>>> Hrm ... Did this pass compile tests?
>>> Likely not, given that generate() is defined as returning int in the
>>> header file and void in the implementation. Rebase issue, or sending the
>>> wrong version ?
>> Indeed, a faulty rebase on my end .. /o\
>>
>> Although, it *did* pass the compile test and I can confirm
>> compiling the patches again (as is this series).
>> It's quite weird. GCC versions are :
>>
>> C compiler for the host machine: cc (gcc 9.3.0 "cc (Ubuntu
>> 9.3.0-10ubuntu2) 9.3.0")
>> C linker for the host machine: cc ld.bfd 2.34
>> C++ compiler for the host machine: c++ (gcc 9.3.0 "c++ (Ubuntu
>> 9.3.0-10ubuntu2) 9.3.0")
>> C++ linker for the host machine: c++ ld.bfd 2.34
> Could it be that the android option is set to false in your build ?
> Here's what I get with gcc 9.3.0.
>
> [24/172] Compiling C++ object 'src/libcamera/4ab8042@@camera at sha/.._android_jpeg_exif.cpp.o'
> FAILED: src/libcamera/4ab8042@@camera at sha/.._android_jpeg_exif.cpp.o
> g++-9.3.0 -Isrc/libcamera/4ab8042@@camera at sha -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -I../../include/android/hardware/libhardware/include -I../../include/android/metadata -I../../include/android/system/core/include -Iinclude/libcamera -I/usr/include/libexif -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ 'src/libcamera/4ab8042@@camera at sha/.._android_jpeg_exif.cpp.o' -MF 'src/libcamera/4ab8042@@camera at sha/.._android_jpeg_exif.cpp.o.d' -o 'src/libcamera/4ab8042@@camera at sha/.._android_jpeg_exif.cpp.o' -c ../../src/android/jpeg/exif.cpp
> In file included from ../../src/android/jpeg/exif.cpp:8:
> ../../src/android/jpeg/exif.h:32:6: error: attribute ignored [-Werror=attributes]
> 32 | int [[nodiscard]] generate();
> | ^
> ../../src/android/jpeg/exif.h:32:6: note: an attribute that appertains to a type-specifier is ignored
> ../../src/android/jpeg/exif.cpp:218:6: error: no declaration matches ‘void Exif::generate()’
> 218 | void Exif::generate()
> | ^~~~
> In file included from ../../src/android/jpeg/exif.cpp:8:
> ../../src/android/jpeg/exif.h:32:21: note: candidate is: ‘int Exif::generate()’
> 32 | int [[nodiscard]] generate();
> | ^~~~~~~~
> ../../src/android/jpeg/exif.h:18:7: note: ‘class Exif’ defined here
> 18 | class Exif
> | ^~~~
> cc1plus: all warnings being treated as errors
Oh damn! In my ./build-libcamera.sh script, The -Dandroid=true had an
spell error- -Danroid=true. The build did warn in the logs(now that I
can read/see) but it didn't "abort" the build which made me think that
it passed, heh! I should pay more attention to meson build logs. :-(
Apologies for the noise.
>>>> Didn't we add a [[nodiscard]] to this call?
>>>>
>>>> Although - if it does fail to generate, I think we'd still expect to
>>>> call into the encoder->encode() and I think the exif.data() would be
>>>> nullptr,0, (which is what we want), as the encoder checks the size to
>>>> determine if it should be added or not.
>>>>
>>>>> +
>>>>> + 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 9d8d9bc..5def7e3 100644
>>>>> --- a/src/android/jpeg/exif.cpp
>>>>> +++ b/src/android/jpeg/exif.cpp
>>>>> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str
>>>>> exif_entry_unref(entry);
>>>>> }
>>>>>
>>>>> -int Exif::generate()
>>>>> +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 = 1;
>>>>> + switch (orientation) {
>>>>> + case 90:
>>>>> + value = 6;
>>>>> + break;
>>>>> + case 180:
>>>>> + value = 3;
>>>>> + break;
>>>>> + case 270:
>>>>> + value = 8;
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
>>>>> +}
>>>>> +
>>>>> +void Exif::generate()
>>>>> {
>>>>> if (exifData_) {
>>>>> free(exifData_);
>>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
>>>>> index 8fb8ffd..6113ca6 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_ }; }
>>>>> int [[nodiscard]] generate();
>>>>>
More information about the libcamera-devel
mailing list