| Message ID | 20220720095812.2380218-2-chenghaoyang@google.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hello Harvey, Thank you for the patch. 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! > 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 | 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(). > */ > -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. > + data_->size = thumbnail_raw_.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..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 ;-) Once reviewed by someone else, I 'll handle the merge so, Reviewed-by: Umang Jain <umang.jain@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);
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@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@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);
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@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@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@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 >
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(). */ -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()); + data_->size = thumbnail_raw_.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..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_; }; 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);
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 | 10 ++++++---- src/android/jpeg/exif.h | 10 ++++++++-- src/android/jpeg/post_processor_jpeg.cpp | 6 ++---- 3 files changed, 16 insertions(+), 10 deletions(-)