[libcamera-devel] [PATCH v2 3/3] android: jpeg: post_processor_jpeg: Embed thumbnail into Exif metadata
Umang Jain
email at uajain.com
Wed Oct 28 07:00:16 CET 2020
Hi Laurent,
Thanks for the review.
On 10/28/20 6:59 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Wed, Oct 28, 2020 at 02:54:47AM +0530, Umang Jain wrote:
>> Embed a Jpeg-encoded thumbnail into Exif metadata using the Thumbnailer
>> class that got introduced.
>>
>> Introduce a helper function in Exif class for setting the thumbnail
>> data. Set the EXIF_TAG_COMPRESSION to '6' to denote that the thumbnail
>> is jpeg-compressed, as mentioned in Exif v2.31.
>>
>> Signed-off-by: Umang Jain <email at uajain.com>
>> ---
>> src/android/jpeg/exif.cpp | 24 ++++++++++++++++-
>> src/android/jpeg/exif.h | 2 ++
>> src/android/jpeg/post_processor_jpeg.cpp | 34 ++++++++++++++++++++++++
>> src/android/jpeg/post_processor_jpeg.h | 8 +++++-
>> 4 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
>> index d21534a..6ac52c6 100644
>> --- a/src/android/jpeg/exif.cpp
>> +++ b/src/android/jpeg/exif.cpp
>> @@ -75,8 +75,16 @@ Exif::~Exif()
>> if (exifData_)
>> free(exifData_);
>>
>> - if (data_)
>> + if (data_) {
>> + /*
>> + * Reset thumbnail data to avoid getting double-freed by
>> + * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
>> + */
>> + data_->data = nullptr;
>> + data_->size = 0;
>> +
>> exif_data_unref(data_);
>> + }
>>
>> if (mem_)
>> exif_mem_unref(mem_);
>> @@ -268,6 +276,20 @@ void Exif::setOrientation(int orientation)
>> setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
>> }
>>
>> +/*
>> + * The thumbnail data should remain valid until the Exif object is destroyed.
>> + * Failing to do so, might result in no thumbnail data being set even after a
>> + * call to Exif::setThumbnail().
>> + */
>> +void Exif::setThumbnail(Span<const unsigned char> thumbnail,
>> + uint16_t compression)
>> +{
>> + data_->data = const_cast<unsigned char *>(thumbnail.data());
>> + data_->size = thumbnail.size();
>> +
>> + setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
>> +}
>> +
>> [[nodiscard]] int Exif::generate()
>> {
>> if (exifData_) {
>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
>> index 12c27b6..6987b31 100644
>> --- a/src/android/jpeg/exif.h
>> +++ b/src/android/jpeg/exif.h
>> @@ -26,6 +26,8 @@ public:
>>
>> void setOrientation(int orientation);
>> void setSize(const libcamera::Size &size);
>> + void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
>> + uint16_t compression);
>> void setTimestamp(time_t timestamp);
>>
>> libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index c56f1b2..a0db793 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -39,11 +39,41 @@ 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;
>> + }
>> +
>> encoder_ = std::make_unique<EncoderLibJpeg>();
>>
>> return encoder_->configure(inCfg);
>> }
>>
>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>> + std::vector<unsigned char> *thumbnail)
>> +{
>> + /* Stores the raw scaled-down thumbnail bytes. */
>> + std::vector<unsigned char> rawThumbnail;
>> +
>> + thumbnailer_.createThumbnail(source, &rawThumbnail);
>> +
>> + if (!rawThumbnail.empty()) {
>> + thumbnail->resize(rawThumbnail.size());
> This will zero the contents of the vector, which isn't required. Let's
> leave it as-is for now, but record the issue:
>
> /*
> * \todo Avoid value-initialization of all elements of the
> * vector.
> */
>
>> +
>> + int jpeg_size = thumbnailEncoder_.encode(rawThumbnail,
>> + *thumbnail, { });
>> + thumbnail->resize(jpeg_size);
>> +
>> + LOG(JPEG, Debug)
>> + << "Thumbnail compress returned "
>> + << jpeg_size << " bytes";
>> + }
>> +}
>> +
>> int PostProcessorJpeg::process(const FrameBuffer &source,
>> Span<uint8_t> destination,
>> CameraMetadata *metadata)
>> @@ -64,6 +94,10 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>> * second, it is good enough.
>> */
>> exif.setTimestamp(std::time(nullptr));
>> + std::vector<unsigned char> thumbnail;
>> + generateThumbnail(source, &thumbnail);
> I think we could return the vector from generateThumbnail() instead of
> passing it from the caller, but that's also not something that needs to
> be addressed now.
>
>> + if(!thumbnail.empty())
>> + exif.setThumbnail(thumbnail, 6);
> The magic value isn't very nice. How about turning the parameter to a
> 'bool compressed', or, better, to an enum declared in the Exif class
>
> enum class Compression {
> None = 1,
> JPEG = 6,
> };
>
> and turning setThumbnail() into
>
> void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
> Compression compression);
>
> Apart from that the patch looks good to me.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> I'll let Kieran have a look too, but I think these small issues could be
> fixed while applying (unless he would prefer you to test them first).
I have applied the changes locally, however I will wait a bit if more
reviews comments flow in, before send another/final version. I can test
the patches too with the cam-file-sink branch, however upto Kieran if he
wants to give quick go at the actual CrOS before pushing.
>
>> if (exif.generate() != 0)
>> LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>>
>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
>> index 3706cec..5afa831 100644
>> --- a/src/android/jpeg/post_processor_jpeg.h
>> +++ b/src/android/jpeg/post_processor_jpeg.h
>> @@ -8,12 +8,13 @@
>> #define __ANDROID_POST_PROCESSOR_JPEG_H__
>>
>> #include "../post_processor.h"
>> +#include "encoder_libjpeg.h"
>> +#include "thumbnailer.h"
>>
>> #include <libcamera/geometry.h>
>>
>> #include "libcamera/internal/buffer.h"
>>
>> -class Encoder;
>> class CameraDevice;
>>
>> class PostProcessorJpeg : public PostProcessor
>> @@ -28,9 +29,14 @@ public:
>> CameraMetadata *metadata) override;
>>
>> private:
>> + void generateThumbnail(const libcamera::FrameBuffer &source,
>> + std::vector<unsigned char> *thumbnail);
>> +
>> CameraDevice *const cameraDevice_;
>> std::unique_ptr<Encoder> encoder_;
>> libcamera::Size streamSize_;
>> + EncoderLibJpeg thumbnailEncoder_;
>> + Thumbnailer thumbnailer_;
>> };
>>
>> #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */
More information about the libcamera-devel
mailing list