[libcamera-devel] [PATCH v6 1/1] Fix Android adaptor thumbnail buffer lifetime

[Cheng-Hao Yang]

Thanks Umang & Laurent!

Followed most of your suggestions and sent the PATCH v2. Please check :)

BR,
Harvey

On Fri, Jul 22, 2022 at 3:30 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hello,
>
> On Thu, Jul 21, 2022 at 11:14:16PM +0530, Umang Jain via libcamera-devel
> wrote:
> > On 7/20/22 15:28, Harvey Yang via libcamera-devel wrote:
> > > Previously the thumbnail buffer is destructed before even being used in
> >
> > Great catch, I think this was the reason for flaky failure for
> > Camera3SimpleStillCaptureTest.JpegExifTest as mentioned!
>
> Oh indeed ! Nice !
>
> > > Exif. This patch moves the buffer into class Exif, so that the
> developer
> > > won't need to worry about its lifetime.
> > >
> > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > ---
> > >   src/android/jpeg/exif.cpp                | 10 ++++++----
> > >   src/android/jpeg/exif.h                  | 10 ++++++++--
> > >   src/android/jpeg/post_processor_jpeg.cpp |  6 ++----
> > >   3 files changed, 16 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > > index 3220b458..584a29d0 100644
> > > --- a/src/android/jpeg/exif.cpp
> > > +++ b/src/android/jpeg/exif.cpp
> > > @@ -435,11 +435,13 @@ void Exif::setOrientation(int orientation)
> > >    * Failing to do so, might result in no thumbnail data being set
> even after a
> > >    * call to Exif::setThumbnail().
> > >    */
>
> Note that the issue is documented here. Clearly that wasn't enough, so
> the API isn't good and needs to be changed. The comment can thus be
> dropped.
>
> > > -void Exif::setThumbnail(Span<const unsigned char> thumbnail,
> > > -                   Compression compression)
> > > +void Exif::setThumbnail(Compression compression)
> > >   {
> > > -   data_->data = const_cast<unsigned char *>(thumbnail.data());
> > > -   data_->size = thumbnail.size();
> > > +   if (thumbnail_raw_.empty())
> > > +           return;
> > > +
> > > +   data_->data = const_cast<unsigned char *>(thumbnail_raw_.data());
> >
> > I think we can do away with the const_cast here? Might not be compatible
> > with previous C++ versions?
> >
> > I quickly compile tested
> >
> > -       data_->data = const_cast<unsigned char *>(thumbnail_raw_.data());
> > +       data_->data = thumbnail_raw_.data();
> >
> > on clang version 12.0.1 and gcc (GCC) 11.0.1 which seems OK.
>
> There should be no need for a const_cast, indeed.
>
> > > +   data_->size = thumbnail_raw_.size();
>
> I would do this a bit differently, to avoid the getThumbnailRaw(), which
> really creates an interdependency between the Exif class and the
> PostProcessorJpeg class.
>
> void Exif::setThumbnail(std::vector<uint8_t> &&data, Compression
> compression)
> {
>         thumbnail_raw_ = std::move(data);
>
>         data_->data = thumbnail_raw_.data();
>         data_->size = thumbnail_raw_.size();
>
>         setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
> }
>
> The only change to the caller compared to the current code would then be
>
> -                               exif.setThumbnail(thumbnail,
> Exif::Compression::JPEG);
> +                               exif.setThumbnail(std::move(thumbnail),
> +                                                 Exif::Compression::JPEG);
>
> > >
> > >     setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
> > >   }
> > > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > > index 2ff8fb78..4784496b 100644
> > > --- a/src/android/jpeg/exif.h
> > > +++ b/src/android/jpeg/exif.h
> > > @@ -10,6 +10,7 @@
> > >   #include <chrono>
> > >   #include <string>
> > >   #include <time.h>
> > > +#include <vector>
> > >
> > >   #include <libexif/exif-data.h>
> > >
> > > @@ -60,8 +61,7 @@ public:
> > >
> > >     void setOrientation(int orientation);
> > >     void setSize(const libcamera::Size &size);
> > > -   void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
> > > -                     Compression compression);
> > > +   void setThumbnail(Compression compression);
> > >     void setTimestamp(time_t timestamp, std::chrono::milliseconds
> msec);
> > >
> > >     void setGPSDateTimestamp(time_t timestamp);
> > > @@ -78,6 +78,10 @@ public:
> > >     libcamera::Span<const uint8_t> data() const { return { exifData_,
> size_ }; }
> > >     [[nodiscard]] int generate();
> > >
> > > +   std::vector<unsigned char>& getThumbnailRaw() {
> > > +           return thumbnail_raw_;
> > > +   }
> > > +
> > >   private:
> > >     ExifEntry *createEntry(ExifIfd ifd, ExifTag tag);
> > >     ExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
> > > @@ -106,4 +110,6 @@ private:
> > >
> > >     unsigned char *exifData_;
> > >     unsigned int size_;
> > > +
> > > +   std::vector<unsigned char> thumbnail_raw_;
> >
> > CamelCase everywhere please ;-)
>
> And I'd call it thumbnailData_, or just thumbnail_.
>
> > Once reviewed by someone else, I 'll handle the merge so,
> >
> > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> >
> > >   };
> > > diff --git a/src/android/jpeg/post_processor_jpeg.cpp
> b/src/android/jpeg/post_processor_jpeg.cpp
> > > index d72ebc3c..506b00b9 100644
> > > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > > @@ -163,10 +163,8 @@ void
> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
> > >             resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY,
> quality);
> > >
> > >             if (thumbnailSize != Size(0, 0)) {
> > > -                   std::vector<unsigned char> thumbnail;
> > > -                   generateThumbnail(source, thumbnailSize, quality,
> &thumbnail);
> > > -                   if (!thumbnail.empty())
> > > -                           exif.setThumbnail(thumbnail,
> Exif::Compression::JPEG);
> > > +                   generateThumbnail(source, thumbnailSize, quality,
> &exif.getThumbnailRaw());
> > > +                   exif.setThumbnail(Exif::Compression::JPEG);
> > >             }
> > >
> > >             resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
> data, 2);
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220722/b9cb4862/attachment.htm>