[libcamera-devel,v2,1/1] Fix Android adaptor thumbnail buffer lifetime
diff mbox series

Message ID 20220722070600.2949278-2-chenghaoyang@google.com
State Accepted
Commit 74794de987c069250deba03c1a55ccd6f659e9e8
Headers show
Series
  • Android adaptor thumbnail buffer lifetime issue
Related show

Commit Message

Harvey Yang July 22, 2022, 7:06 a.m. UTC 
Previously the thumbnail buffer is destructed before even being used in
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@chromium.org>
---
 src/android/jpeg/exif.cpp                | 13 +++++--------
 src/android/jpeg/exif.h                  |  5 ++++-
 src/android/jpeg/post_processor_jpeg.cpp |  2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

Comments

Laurent Pinchart July 22, 2022, 8:29 a.m. UTC | #1 
Hi Harvey,

Thank you for the patch.

On Fri, Jul 22, 2022 at 07:06:00AM +0000, Harvey Yang via libcamera-devel wrote:
> Previously the thumbnail buffer is destructed before even being used in
> 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@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/android/jpeg/exif.cpp                | 13 +++++--------
>  src/android/jpeg/exif.h                  |  5 ++++-
>  src/android/jpeg/post_processor_jpeg.cpp |  2 +-
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index 3220b458..6b1d0f1f 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -430,16 +430,13 @@ 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,
> +void Exif::setThumbnail(std::vector<unsigned char> &&thumbnail,
>  			Compression compression)
>  {
> -	data_->data = const_cast<unsigned char *>(thumbnail.data());
> -	data_->size = thumbnail.size();
> +	thumbnailData_ = std::move(thumbnail);
> +
> +	data_->data = thumbnailData_.data();
> +	data_->size = thumbnailData_.size();
>  
>  	setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
>  }
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> index 2ff8fb78..e68716f3 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,7 +61,7 @@ public:
>  
>  	void setOrientation(int orientation);
>  	void setSize(const libcamera::Size &size);
> -	void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
> +	void setThumbnail(std::vector<unsigned char> &&thumbnail,
>  			  Compression compression);
>  	void setTimestamp(time_t timestamp, std::chrono::milliseconds msec);
>  
> @@ -106,4 +107,6 @@ private:
>  
>  	unsigned char *exifData_;
>  	unsigned int size_;
> +
> +	std::vector<unsigned char> thumbnailData_;
>  };
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index d72ebc3c..0cf56716 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -166,7 +166,7 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
>  			std::vector<unsigned char> thumbnail;
>  			generateThumbnail(source, thumbnailSize, quality, &thumbnail);
>  			if (!thumbnail.empty())
> -				exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
> +				exif.setThumbnail(std::move(thumbnail), Exif::Compression::JPEG);
>  		}
>  
>  		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
Umang Jain July 22, 2022, 12:43 p.m. UTC | #2 
Hello,

Thank you for other version.

I would re-write the subject line with:

     android: exif: Fix thumbnail buffer lifetime

It can be handled during integration, no need for v3.

On 7/22/22 13:59, Laurent Pinchart via libcamera-devel wrote:
> Hi Harvey,
>
> Thank you for the patch.
>
> On Fri, Jul 22, 2022 at 07:06:00AM +0000, Harvey Yang via libcamera-devel wrote:
>> Previously the thumbnail buffer is destructed before even being used in
>> 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@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Again,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

>
>> ---
>>   src/android/jpeg/exif.cpp                | 13 +++++--------
>>   src/android/jpeg/exif.h                  |  5 ++++-
>>   src/android/jpeg/post_processor_jpeg.cpp |  2 +-
>>   3 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
>> index 3220b458..6b1d0f1f 100644
>> --- a/src/android/jpeg/exif.cpp
>> +++ b/src/android/jpeg/exif.cpp
>> @@ -430,16 +430,13 @@ 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,
>> +void Exif::setThumbnail(std::vector<unsigned char> &&thumbnail,
>>   			Compression compression)
>>   {
>> -	data_->data = const_cast<unsigned char *>(thumbnail.data());
>> -	data_->size = thumbnail.size();
>> +	thumbnailData_ = std::move(thumbnail);
>> +
>> +	data_->data = thumbnailData_.data();
>> +	data_->size = thumbnailData_.size();
>>   
>>   	setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
>>   }
>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
>> index 2ff8fb78..e68716f3 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,7 +61,7 @@ public:
>>   
>>   	void setOrientation(int orientation);
>>   	void setSize(const libcamera::Size &size);
>> -	void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
>> +	void setThumbnail(std::vector<unsigned char> &&thumbnail,
>>   			  Compression compression);
>>   	void setTimestamp(time_t timestamp, std::chrono::milliseconds msec);
>>   
>> @@ -106,4 +107,6 @@ private:
>>   
>>   	unsigned char *exifData_;
>>   	unsigned int size_;
>> +
>> +	std::vector<unsigned char> thumbnailData_;
>>   };
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index d72ebc3c..0cf56716 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -166,7 +166,7 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
>>   			std::vector<unsigned char> thumbnail;
>>   			generateThumbnail(source, thumbnailSize, quality, &thumbnail);
>>   			if (!thumbnail.empty())
>> -				exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
>> +				exif.setThumbnail(std::move(thumbnail), Exif::Compression::JPEG);
>>   		}
>>   
>>   		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
Harvey Yang July 22, 2022, 4:04 p.m. UTC | #3 
Sure and thanks, Umang!

BR,
Harvey

On Fri, Jul 22, 2022 at 8:43 PM Umang Jain <umang.jain@ideasonboard.com>
wrote:

> Hello,
>
> Thank you for other version.
>
> I would re-write the subject line with:
>
>      android: exif: Fix thumbnail buffer lifetime
>
> It can be handled during integration, no need for v3.
>
> On 7/22/22 13:59, Laurent Pinchart via libcamera-devel wrote:
> > Hi Harvey,
> >
> > Thank you for the patch.
> >
> > On Fri, Jul 22, 2022 at 07:06:00AM +0000, Harvey Yang via
> libcamera-devel wrote:
> >> Previously the thumbnail buffer is destructed before even being used in
> >> 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@chromium.org>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Again,
>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>
> >
> >> ---
> >>   src/android/jpeg/exif.cpp                | 13 +++++--------
> >>   src/android/jpeg/exif.h                  |  5 ++++-
> >>   src/android/jpeg/post_processor_jpeg.cpp |  2 +-
> >>   3 files changed, 10 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> >> index 3220b458..6b1d0f1f 100644
> >> --- a/src/android/jpeg/exif.cpp
> >> +++ b/src/android/jpeg/exif.cpp
> >> @@ -430,16 +430,13 @@ 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,
> >> +void Exif::setThumbnail(std::vector<unsigned char> &&thumbnail,
> >>                      Compression compression)
> >>   {
> >> -    data_->data = const_cast<unsigned char *>(thumbnail.data());
> >> -    data_->size = thumbnail.size();
> >> +    thumbnailData_ = std::move(thumbnail);
> >> +
> >> +    data_->data = thumbnailData_.data();
> >> +    data_->size = thumbnailData_.size();
> >>
> >>      setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
> >>   }
> >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> >> index 2ff8fb78..e68716f3 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,7 +61,7 @@ public:
> >>
> >>      void setOrientation(int orientation);
> >>      void setSize(const libcamera::Size &size);
> >> -    void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
> >> +    void setThumbnail(std::vector<unsigned char> &&thumbnail,
> >>                        Compression compression);
> >>      void setTimestamp(time_t timestamp, std::chrono::milliseconds
> msec);
> >>
> >> @@ -106,4 +107,6 @@ private:
> >>
> >>      unsigned char *exifData_;
> >>      unsigned int size_;
> >> +
> >> +    std::vector<unsigned char> thumbnailData_;
> >>   };
> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp
> b/src/android/jpeg/post_processor_jpeg.cpp
> >> index d72ebc3c..0cf56716 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >> @@ -166,7 +166,7 @@ void
> PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
> >>                      std::vector<unsigned char> thumbnail;
> >>                      generateThumbnail(source, thumbnailSize, quality,
> &thumbnail);
> >>                      if (!thumbnail.empty())
> >> -                            exif.setThumbnail(thumbnail,
> Exif::Compression::JPEG);
> >> +                            exif.setThumbnail(std::move(thumbnail),
> Exif::Compression::JPEG);
> >>              }
> >>
> >>              resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
> data, 2);
>

Patch
diff mbox series 

diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
index 3220b458..6b1d0f1f 100644
--- a/src/android/jpeg/exif.cpp
+++ b/src/android/jpeg/exif.cpp
@@ -430,16 +430,13 @@  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,
+void Exif::setThumbnail(std::vector<unsigned char> &&thumbnail,
 			Compression compression)
 {
-	data_->data = const_cast<unsigned char *>(thumbnail.data());
-	data_->size = thumbnail.size();
+	thumbnailData_ = std::move(thumbnail);
+
+	data_->data = thumbnailData_.data();
+	data_->size = thumbnailData_.size();
 
 	setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
 }
diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
index 2ff8fb78..e68716f3 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,7 +61,7 @@  public:
 
 	void setOrientation(int orientation);
 	void setSize(const libcamera::Size &size);
-	void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
+	void setThumbnail(std::vector<unsigned char> &&thumbnail,
 			  Compression compression);
 	void setTimestamp(time_t timestamp, std::chrono::milliseconds msec);
 
@@ -106,4 +107,6 @@  private:
 
 	unsigned char *exifData_;
 	unsigned int size_;
+
+	std::vector<unsigned char> thumbnailData_;
 };
diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
index d72ebc3c..0cf56716 100644
--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
@@ -166,7 +166,7 @@  void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
 			std::vector<unsigned char> thumbnail;
 			generateThumbnail(source, thumbnailSize, quality, &thumbnail);
 			if (!thumbnail.empty())
-				exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
+				exif.setThumbnail(std::move(thumbnail), Exif::Compression::JPEG);
 		}
 
 		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);