| Message ID | 20201020074229.119151-2-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thanks for your work. On 10/20/20 1:12 PM, Hirokazu Honda wrote: > In Encoder::encode(), the |source| argument doesn't have to be a > pointer. This replaces its type, const pointer, with const > reference as the latter is preferred to the former. > libcamera::Span is chea to construct/copy/move. We should deal s/chea/cheap > with the type as pass-by-value parameter. Therefore this also > drops the const reference in the |destination| argument. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> LGTM again. Reviewed-by: Umang Jain <email@uajain.com> > --- > src/android/jpeg/encoder.h | 4 ++-- > src/android/jpeg/encoder_libjpeg.cpp | 6 +++--- > src/android/jpeg/encoder_libjpeg.h | 4 ++-- > src/android/jpeg/post_processor_jpeg.cpp | 2 +- > 4 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h > index 4483153..4fe71cf 100644 > --- a/src/android/jpeg/encoder.h > +++ b/src/android/jpeg/encoder.h > @@ -17,8 +17,8 @@ public: > virtual ~Encoder() {}; > > virtual int configure(const libcamera::StreamConfiguration &cfg) = 0; > - virtual int encode(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > + virtual int encode(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > const libcamera::Span<const uint8_t> &exifData) = 0; > }; > > diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp > index 8995ba5..4236c9d 100644 > --- a/src/android/jpeg/encoder_libjpeg.cpp > +++ b/src/android/jpeg/encoder_libjpeg.cpp > @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame) > } > } > > -int EncoderLibJpeg::encode(const FrameBuffer *source, > - const libcamera::Span<uint8_t> &dest, > +int EncoderLibJpeg::encode(const FrameBuffer &source, > + libcamera::Span<uint8_t> dest, > const libcamera::Span<const uint8_t> &exifData) > { > - MappedFrameBuffer frame(source, PROT_READ); > + MappedFrameBuffer frame(&source, PROT_READ); > if (!frame.isValid()) { > LOG(JPEG, Error) << "Failed to map FrameBuffer : " > << strerror(frame.error()); > diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h > index 934caef..391a53c 100644 > --- a/src/android/jpeg/encoder_libjpeg.h > +++ b/src/android/jpeg/encoder_libjpeg.h > @@ -21,8 +21,8 @@ public: > ~EncoderLibJpeg(); > > int configure(const libcamera::StreamConfiguration &cfg) override; > - int encode(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > + int encode(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > const libcamera::Span<const uint8_t> &exifData) override; > > private: > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 55b9088..6f33631 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, > if (exif.generate() != 0) > LOG(JPEG, Error) << "Failed to generate valid EXIF data"; > > - int jpeg_size = encoder_->encode(&source, destination, exif.data()); > + int jpeg_size = encoder_->encode(source, destination, exif.data()); Perfect. I was expecting this from v1. > if (jpeg_size < 0) { > LOG(JPEG, Error) << "Failed to encode stream image"; > return jpeg_size;
Hi Hiro, On 20/10/2020 11:38, Umang Jain wrote: > Hi Hiro, > > Thanks for your work. > > On 10/20/20 1:12 PM, Hirokazu Honda wrote: >> In Encoder::encode(), the |source| argument doesn't have to be a >> pointer. This replaces its type, const pointer, with const >> reference as the latter is preferred to the former. >> libcamera::Span is chea to construct/copy/move. We should deal > s/chea/cheap Hrm, this is fine in the original. Must have caught it while editing. >> with the type as pass-by-value parameter. Therefore this also >> drops the const reference in the |destination| argument. >> >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > LGTM again. > Reviewed-by: Umang Jain <email@uajain.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/android/jpeg/encoder.h | 4 ++-- >> src/android/jpeg/encoder_libjpeg.cpp | 6 +++--- >> src/android/jpeg/encoder_libjpeg.h | 4 ++-- >> src/android/jpeg/post_processor_jpeg.cpp | 2 +- >> 4 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h >> index 4483153..4fe71cf 100644 >> --- a/src/android/jpeg/encoder.h >> +++ b/src/android/jpeg/encoder.h >> @@ -17,8 +17,8 @@ public: >> virtual ~Encoder() {}; >> virtual int configure(const libcamera::StreamConfiguration >> &cfg) = 0; >> - virtual int encode(const libcamera::FrameBuffer *source, >> - const libcamera::Span<uint8_t> &destination, >> + virtual int encode(const libcamera::FrameBuffer &source, >> + libcamera::Span<uint8_t> destination, >> const libcamera::Span<const uint8_t> &exifData) = 0; >> }; >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp >> b/src/android/jpeg/encoder_libjpeg.cpp >> index 8995ba5..4236c9d 100644 >> --- a/src/android/jpeg/encoder_libjpeg.cpp >> +++ b/src/android/jpeg/encoder_libjpeg.cpp >> @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const >> libcamera::MappedBuffer *frame) >> } >> } >> -int EncoderLibJpeg::encode(const FrameBuffer *source, >> - const libcamera::Span<uint8_t> &dest, >> +int EncoderLibJpeg::encode(const FrameBuffer &source, >> + libcamera::Span<uint8_t> dest, >> const libcamera::Span<const uint8_t> &exifData) >> { >> - MappedFrameBuffer frame(source, PROT_READ); >> + MappedFrameBuffer frame(&source, PROT_READ); Perhaps MappedFrameBuffer should also take a reference, but lets not worry about that here. >> if (!frame.isValid()) { >> LOG(JPEG, Error) << "Failed to map FrameBuffer : " >> << strerror(frame.error()); >> diff --git a/src/android/jpeg/encoder_libjpeg.h >> b/src/android/jpeg/encoder_libjpeg.h >> index 934caef..391a53c 100644 >> --- a/src/android/jpeg/encoder_libjpeg.h >> +++ b/src/android/jpeg/encoder_libjpeg.h >> @@ -21,8 +21,8 @@ public: >> ~EncoderLibJpeg(); >> int configure(const libcamera::StreamConfiguration &cfg) >> override; >> - int encode(const libcamera::FrameBuffer *source, >> - const libcamera::Span<uint8_t> &destination, >> + int encode(const libcamera::FrameBuffer &source, >> + libcamera::Span<uint8_t> destination, >> const libcamera::Span<const uint8_t> &exifData) override; >> private: >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp >> b/src/android/jpeg/post_processor_jpeg.cpp >> index 55b9088..6f33631 100644 >> --- a/src/android/jpeg/post_processor_jpeg.cpp >> +++ b/src/android/jpeg/post_processor_jpeg.cpp >> @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const >> libcamera::FrameBuffer &source, >> if (exif.generate() != 0) >> LOG(JPEG, Error) << "Failed to generate valid EXIF data"; >> - int jpeg_size = encoder_->encode(&source, destination, >> exif.data()); >> + int jpeg_size = encoder_->encode(source, destination, exif.data()); > Perfect. I was expecting this from v1. >> if (jpeg_size < 0) { >> LOG(JPEG, Error) << "Failed to encode stream image"; >> return jpeg_size; > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On 20/10/2020 15:57, Kieran Bingham wrote: > Hi Hiro, > > On 20/10/2020 11:38, Umang Jain wrote: >> Hi Hiro, >> >> Thanks for your work. >> >> On 10/20/20 1:12 PM, Hirokazu Honda wrote: >>> In Encoder::encode(), the |source| argument doesn't have to be a >>> pointer. This replaces its type, const pointer, with const >>> reference as the latter is preferred to the former. >>> libcamera::Span is chea to construct/copy/move. We should deal >> s/chea/cheap > > Hrm, this is fine in the original. Must have caught it while editing. Ignore that - I was comparing against a different patch ;-) Fixed up here locally. > >>> with the type as pass-by-value parameter. Therefore this also >>> drops the const reference in the |destination| argument. >>> >>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> >> LGTM again. >> Reviewed-by: Umang Jain <email@uajain.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- >>> src/android/jpeg/encoder.h | 4 ++-- >>> src/android/jpeg/encoder_libjpeg.cpp | 6 +++--- >>> src/android/jpeg/encoder_libjpeg.h | 4 ++-- >>> src/android/jpeg/post_processor_jpeg.cpp | 2 +- >>> 4 files changed, 8 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h >>> index 4483153..4fe71cf 100644 >>> --- a/src/android/jpeg/encoder.h >>> +++ b/src/android/jpeg/encoder.h >>> @@ -17,8 +17,8 @@ public: >>> virtual ~Encoder() {}; >>> virtual int configure(const libcamera::StreamConfiguration >>> &cfg) = 0; >>> - virtual int encode(const libcamera::FrameBuffer *source, >>> - const libcamera::Span<uint8_t> &destination, >>> + virtual int encode(const libcamera::FrameBuffer &source, >>> + libcamera::Span<uint8_t> destination, >>> const libcamera::Span<const uint8_t> &exifData) = 0; >>> }; >>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp >>> b/src/android/jpeg/encoder_libjpeg.cpp >>> index 8995ba5..4236c9d 100644 >>> --- a/src/android/jpeg/encoder_libjpeg.cpp >>> +++ b/src/android/jpeg/encoder_libjpeg.cpp >>> @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const >>> libcamera::MappedBuffer *frame) >>> } >>> } >>> -int EncoderLibJpeg::encode(const FrameBuffer *source, >>> - const libcamera::Span<uint8_t> &dest, >>> +int EncoderLibJpeg::encode(const FrameBuffer &source, >>> + libcamera::Span<uint8_t> dest, >>> const libcamera::Span<const uint8_t> &exifData) >>> { >>> - MappedFrameBuffer frame(source, PROT_READ); >>> + MappedFrameBuffer frame(&source, PROT_READ); > > Perhaps MappedFrameBuffer should also take a reference, but lets not > worry about that here. > >>> if (!frame.isValid()) { >>> LOG(JPEG, Error) << "Failed to map FrameBuffer : " >>> << strerror(frame.error()); >>> diff --git a/src/android/jpeg/encoder_libjpeg.h >>> b/src/android/jpeg/encoder_libjpeg.h >>> index 934caef..391a53c 100644 >>> --- a/src/android/jpeg/encoder_libjpeg.h >>> +++ b/src/android/jpeg/encoder_libjpeg.h >>> @@ -21,8 +21,8 @@ public: >>> ~EncoderLibJpeg(); >>> int configure(const libcamera::StreamConfiguration &cfg) >>> override; >>> - int encode(const libcamera::FrameBuffer *source, >>> - const libcamera::Span<uint8_t> &destination, >>> + int encode(const libcamera::FrameBuffer &source, >>> + libcamera::Span<uint8_t> destination, >>> const libcamera::Span<const uint8_t> &exifData) override; >>> private: >>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp >>> b/src/android/jpeg/post_processor_jpeg.cpp >>> index 55b9088..6f33631 100644 >>> --- a/src/android/jpeg/post_processor_jpeg.cpp >>> +++ b/src/android/jpeg/post_processor_jpeg.cpp >>> @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const >>> libcamera::FrameBuffer &source, >>> if (exif.generate() != 0) >>> LOG(JPEG, Error) << "Failed to generate valid EXIF data"; >>> - int jpeg_size = encoder_->encode(&source, destination, >>> exif.data()); >>> + int jpeg_size = encoder_->encode(source, destination, exif.data()); >> Perfect. I was expecting this from v1. >>> if (jpeg_size < 0) { >>> LOG(JPEG, Error) << "Failed to encode stream image"; >>> return jpeg_size; >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h index 4483153..4fe71cf 100644 --- a/src/android/jpeg/encoder.h +++ b/src/android/jpeg/encoder.h @@ -17,8 +17,8 @@ public: virtual ~Encoder() {}; virtual int configure(const libcamera::StreamConfiguration &cfg) = 0; - virtual int encode(const libcamera::FrameBuffer *source, - const libcamera::Span<uint8_t> &destination, + virtual int encode(const libcamera::FrameBuffer &source, + libcamera::Span<uint8_t> destination, const libcamera::Span<const uint8_t> &exifData) = 0; }; diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp index 8995ba5..4236c9d 100644 --- a/src/android/jpeg/encoder_libjpeg.cpp +++ b/src/android/jpeg/encoder_libjpeg.cpp @@ -179,11 +179,11 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame) } } -int EncoderLibJpeg::encode(const FrameBuffer *source, - const libcamera::Span<uint8_t> &dest, +int EncoderLibJpeg::encode(const FrameBuffer &source, + libcamera::Span<uint8_t> dest, const libcamera::Span<const uint8_t> &exifData) { - MappedFrameBuffer frame(source, PROT_READ); + MappedFrameBuffer frame(&source, PROT_READ); if (!frame.isValid()) { LOG(JPEG, Error) << "Failed to map FrameBuffer : " << strerror(frame.error()); diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h index 934caef..391a53c 100644 --- a/src/android/jpeg/encoder_libjpeg.h +++ b/src/android/jpeg/encoder_libjpeg.h @@ -21,8 +21,8 @@ public: ~EncoderLibJpeg(); int configure(const libcamera::StreamConfiguration &cfg) override; - int encode(const libcamera::FrameBuffer *source, - const libcamera::Span<uint8_t> &destination, + int encode(const libcamera::FrameBuffer &source, + libcamera::Span<uint8_t> destination, const libcamera::Span<const uint8_t> &exifData) override; private: diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 55b9088..6f33631 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -67,7 +67,7 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, if (exif.generate() != 0) LOG(JPEG, Error) << "Failed to generate valid EXIF data"; - int jpeg_size = encoder_->encode(&source, destination, exif.data()); + int jpeg_size = encoder_->encode(source, destination, exif.data()); if (jpeg_size < 0) { LOG(JPEG, Error) << "Failed to encode stream image"; return jpeg_size;
In Encoder::encode(), the |source| argument doesn't have to be a pointer. This replaces its type, const pointer, with const reference as the latter is preferred to the former. libcamera::Span is chea to construct/copy/move. We should deal with the type as pass-by-value parameter. Therefore this also drops the const reference in the |destination| argument. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/android/jpeg/encoder.h | 4 ++-- src/android/jpeg/encoder_libjpeg.cpp | 6 +++--- src/android/jpeg/encoder_libjpeg.h | 4 ++-- src/android/jpeg/post_processor_jpeg.cpp | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-)