| Message ID | 20200924095008.338920-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On 9/24/20 3:20 PM, Kieran Bingham wrote: > The exif object sets the byte ordering on construction, and then > during later calls re-states the byte ordering when setting values. > > It could be argued that this ordering should already be known to the exif > library and is redudant, but even so we must provide it. > > Ensure we are consistent in always using the same byte ordering by setting > a private class member to re-use a single value. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > This patch itself feels like it might be a bit redundant, but I saw the > repetition, and whilst we're not likely to change the byte order, I > thought it might make sense to ensure it's always set from the same > state variable. > > If this is helpful we can add it, but if this is overkill I don't mind > dropping it. It's just an idea. It is indeed helpful and the idea has crossed my mind in the past but you got to it first. :) So, fine by me, Reviewed-by: Umang Jain <email@uajain.com> > > src/android/jpeg/exif.cpp | 11 ++++++----- > src/android/jpeg/exif.h | 1 + > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > index 1ced55343ee9..f29a7c71d1ec 100644 > --- a/src/android/jpeg/exif.cpp > +++ b/src/android/jpeg/exif.cpp > @@ -25,7 +25,8 @@ LOG_DEFINE_CATEGORY(EXIF) > * data can be obtained using the data() method. > */ > Exif::Exif() > - : valid_(false), data_(nullptr), exifData_(0), size_(0) > + : valid_(false), data_(nullptr), order_(EXIF_BYTE_ORDER_INTEL), > + exifData_(0), size_(0) > { > /* Create an ExifMem allocator to construct entries. */ > mem_ = exif_mem_new_default(); > @@ -49,7 +50,7 @@ Exif::Exif() > * Big-Endian: EXIF_BYTE_ORDER_MOTOROLA > * Little Endian: EXIF_BYTE_ORDER_INTEL > */ > - exif_data_set_byte_order(data_, EXIF_BYTE_ORDER_INTEL); > + exif_data_set_byte_order(data_, order_); > > /* Create the mandatory EXIF fields with default data. */ > exif_data_fix(data_); > @@ -131,7 +132,7 @@ void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item) > if (!entry) > return; > > - exif_set_short(entry->data, EXIF_BYTE_ORDER_INTEL, item); > + exif_set_short(entry->data, order_, item); > exif_entry_unref(entry); > } > > @@ -141,7 +142,7 @@ void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item) > if (!entry) > return; > > - exif_set_long(entry->data, EXIF_BYTE_ORDER_INTEL, item); > + exif_set_long(entry->data, order_, item); > exif_entry_unref(entry); > } > > @@ -151,7 +152,7 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item) > if (!entry) > return; > > - exif_set_rational(entry->data, EXIF_BYTE_ORDER_INTEL, item); > + exif_set_rational(entry->data, order_, item); > exif_entry_unref(entry); > } > > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > index 622de4cfd593..6e8ce04a2e67 100644 > --- a/src/android/jpeg/exif.h > +++ b/src/android/jpeg/exif.h > @@ -46,6 +46,7 @@ private: > > ExifData *data_; > ExifMem *mem_; > + ExifByteOrder order_; > > unsigned char *exifData_; > unsigned int size_;
Hi, Can we get additional R-b tags to merge these trivial changes? Thanks. On 9/24/20 3:20 PM, Kieran Bingham wrote: > The exif object sets the byte ordering on construction, and then > during later calls re-states the byte ordering when setting values. > > It could be argued that this ordering should already be known to the exif > library and is redudant, but even so we must provide it. > > Ensure we are consistent in always using the same byte ordering by setting > a private class member to re-use a single value. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > This patch itself feels like it might be a bit redundant, but I saw the > repetition, and whilst we're not likely to change the byte order, I > thought it might make sense to ensure it's always set from the same > state variable. > > If this is helpful we can add it, but if this is overkill I don't mind > dropping it. It's just an idea. > > src/android/jpeg/exif.cpp | 11 ++++++----- > src/android/jpeg/exif.h | 1 + > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > index 1ced55343ee9..f29a7c71d1ec 100644 > --- a/src/android/jpeg/exif.cpp > +++ b/src/android/jpeg/exif.cpp > @@ -25,7 +25,8 @@ LOG_DEFINE_CATEGORY(EXIF) > * data can be obtained using the data() method. > */ > Exif::Exif() > - : valid_(false), data_(nullptr), exifData_(0), size_(0) > + : valid_(false), data_(nullptr), order_(EXIF_BYTE_ORDER_INTEL), > + exifData_(0), size_(0) > { > /* Create an ExifMem allocator to construct entries. */ > mem_ = exif_mem_new_default(); > @@ -49,7 +50,7 @@ Exif::Exif() > * Big-Endian: EXIF_BYTE_ORDER_MOTOROLA > * Little Endian: EXIF_BYTE_ORDER_INTEL > */ > - exif_data_set_byte_order(data_, EXIF_BYTE_ORDER_INTEL); > + exif_data_set_byte_order(data_, order_); > > /* Create the mandatory EXIF fields with default data. */ > exif_data_fix(data_); > @@ -131,7 +132,7 @@ void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item) > if (!entry) > return; > > - exif_set_short(entry->data, EXIF_BYTE_ORDER_INTEL, item); > + exif_set_short(entry->data, order_, item); > exif_entry_unref(entry); > } > > @@ -141,7 +142,7 @@ void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item) > if (!entry) > return; > > - exif_set_long(entry->data, EXIF_BYTE_ORDER_INTEL, item); > + exif_set_long(entry->data, order_, item); > exif_entry_unref(entry); > } > > @@ -151,7 +152,7 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item) > if (!entry) > return; > > - exif_set_rational(entry->data, EXIF_BYTE_ORDER_INTEL, item); > + exif_set_rational(entry->data, order_, item); > exif_entry_unref(entry); > } > > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h > index 622de4cfd593..6e8ce04a2e67 100644 > --- a/src/android/jpeg/exif.h > +++ b/src/android/jpeg/exif.h > @@ -46,6 +46,7 @@ private: > > ExifData *data_; > ExifMem *mem_; > + ExifByteOrder order_; > > unsigned char *exifData_; > unsigned int size_;
Hi Umang, On 02/10/2020 06:03, Umang Jain wrote: > Hi, > > Can we get additional R-b tags to merge these trivial changes? > Thanks. I'm happy, you're happy, it's trivial. I'll merge it. Thanks. Kieran > > On 9/24/20 3:20 PM, Kieran Bingham wrote: >> The exif object sets the byte ordering on construction, and then >> during later calls re-states the byte ordering when setting values. >> >> It could be argued that this ordering should already be known to the exif >> library and is redudant, but even so we must provide it. >> >> Ensure we are consistent in always using the same byte ordering by >> setting >> a private class member to re-use a single value. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> >> This patch itself feels like it might be a bit redundant, but I saw the >> repetition, and whilst we're not likely to change the byte order, I >> thought it might make sense to ensure it's always set from the same >> state variable. >> >> If this is helpful we can add it, but if this is overkill I don't mind >> dropping it. It's just an idea. >> >> src/android/jpeg/exif.cpp | 11 ++++++----- >> src/android/jpeg/exif.h | 1 + >> 2 files changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp >> index 1ced55343ee9..f29a7c71d1ec 100644 >> --- a/src/android/jpeg/exif.cpp >> +++ b/src/android/jpeg/exif.cpp >> @@ -25,7 +25,8 @@ LOG_DEFINE_CATEGORY(EXIF) >> * data can be obtained using the data() method. >> */ >> Exif::Exif() >> - : valid_(false), data_(nullptr), exifData_(0), size_(0) >> + : valid_(false), data_(nullptr), order_(EXIF_BYTE_ORDER_INTEL), >> + exifData_(0), size_(0) >> { >> /* Create an ExifMem allocator to construct entries. */ >> mem_ = exif_mem_new_default(); >> @@ -49,7 +50,7 @@ Exif::Exif() >> * Big-Endian: EXIF_BYTE_ORDER_MOTOROLA >> * Little Endian: EXIF_BYTE_ORDER_INTEL >> */ >> - exif_data_set_byte_order(data_, EXIF_BYTE_ORDER_INTEL); >> + exif_data_set_byte_order(data_, order_); >> /* Create the mandatory EXIF fields with default data. */ >> exif_data_fix(data_); >> @@ -131,7 +132,7 @@ void Exif::setShort(ExifIfd ifd, ExifTag tag, >> uint16_t item) >> if (!entry) >> return; >> - exif_set_short(entry->data, EXIF_BYTE_ORDER_INTEL, item); >> + exif_set_short(entry->data, order_, item); >> exif_entry_unref(entry); >> } >> @@ -141,7 +142,7 @@ void Exif::setLong(ExifIfd ifd, ExifTag tag, >> uint32_t item) >> if (!entry) >> return; >> - exif_set_long(entry->data, EXIF_BYTE_ORDER_INTEL, item); >> + exif_set_long(entry->data, order_, item); >> exif_entry_unref(entry); >> } >> @@ -151,7 +152,7 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, >> ExifRational item) >> if (!entry) >> return; >> - exif_set_rational(entry->data, EXIF_BYTE_ORDER_INTEL, item); >> + exif_set_rational(entry->data, order_, item); >> exif_entry_unref(entry); >> } >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h >> index 622de4cfd593..6e8ce04a2e67 100644 >> --- a/src/android/jpeg/exif.h >> +++ b/src/android/jpeg/exif.h >> @@ -46,6 +46,7 @@ private: >> ExifData *data_; >> ExifMem *mem_; >> + ExifByteOrder order_; >> unsigned char *exifData_; >> unsigned int size_; >
diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp index 1ced55343ee9..f29a7c71d1ec 100644 --- a/src/android/jpeg/exif.cpp +++ b/src/android/jpeg/exif.cpp @@ -25,7 +25,8 @@ LOG_DEFINE_CATEGORY(EXIF) * data can be obtained using the data() method. */ Exif::Exif() - : valid_(false), data_(nullptr), exifData_(0), size_(0) + : valid_(false), data_(nullptr), order_(EXIF_BYTE_ORDER_INTEL), + exifData_(0), size_(0) { /* Create an ExifMem allocator to construct entries. */ mem_ = exif_mem_new_default(); @@ -49,7 +50,7 @@ Exif::Exif() * Big-Endian: EXIF_BYTE_ORDER_MOTOROLA * Little Endian: EXIF_BYTE_ORDER_INTEL */ - exif_data_set_byte_order(data_, EXIF_BYTE_ORDER_INTEL); + exif_data_set_byte_order(data_, order_); /* Create the mandatory EXIF fields with default data. */ exif_data_fix(data_); @@ -131,7 +132,7 @@ void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item) if (!entry) return; - exif_set_short(entry->data, EXIF_BYTE_ORDER_INTEL, item); + exif_set_short(entry->data, order_, item); exif_entry_unref(entry); } @@ -141,7 +142,7 @@ void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item) if (!entry) return; - exif_set_long(entry->data, EXIF_BYTE_ORDER_INTEL, item); + exif_set_long(entry->data, order_, item); exif_entry_unref(entry); } @@ -151,7 +152,7 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item) if (!entry) return; - exif_set_rational(entry->data, EXIF_BYTE_ORDER_INTEL, item); + exif_set_rational(entry->data, order_, item); exif_entry_unref(entry); } diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h index 622de4cfd593..6e8ce04a2e67 100644 --- a/src/android/jpeg/exif.h +++ b/src/android/jpeg/exif.h @@ -46,6 +46,7 @@ private: ExifData *data_; ExifMem *mem_; + ExifByteOrder order_; unsigned char *exifData_; unsigned int size_;
The exif object sets the byte ordering on construction, and then during later calls re-states the byte ordering when setting values. It could be argued that this ordering should already be known to the exif library and is redudant, but even so we must provide it. Ensure we are consistent in always using the same byte ordering by setting a private class member to re-use a single value. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- This patch itself feels like it might be a bit redundant, but I saw the repetition, and whilst we're not likely to change the byte order, I thought it might make sense to ensure it's always set from the same state variable. If this is helpful we can add it, but if this is overkill I don't mind dropping it. It's just an idea. src/android/jpeg/exif.cpp | 11 ++++++----- src/android/jpeg/exif.h | 1 + 2 files changed, 7 insertions(+), 5 deletions(-)