| Message ID | 20201020074229.119151-1-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On 10/20/20 1:12 PM, Hirokazu Honda wrote: > In PostProcessor::process(), the |source| argument doesn't have > to be a pointer. This replace its type, const pointer, with const > reference as the latter is preferred to the former. > libcamera::Span is cheap 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> This looks good to me. Reviewed-by: Umang Jain <email@uajain.com> > --- > src/android/camera_stream.cpp | 2 +- > src/android/jpeg/post_processor_jpeg.cpp | 6 +++--- > src/android/jpeg/post_processor_jpeg.h | 4 ++-- > src/android/post_processor.h | 4 ++-- > 4 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index 1b8afa8..28a6e09 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -102,7 +102,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source, > if (!postProcessor_) > return 0; > > - return postProcessor_->process(&source, dest->maps()[0], metadata); > + return postProcessor_->process(source, dest->maps()[0], metadata); > } > > FrameBuffer *CameraStream::getBuffer() > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 753c28e..55b9088 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg, > return encoder_->configure(inCfg); > } > > -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > CameraMetadata *metadata) > { > if (!encoder_) > @@ -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()); I see that this change is required to keep the patch compile-able because the argument's type has been changed. I would expect this would be reworked in next patches while correctly the encoder interface. > if (jpeg_size < 0) { > LOG(JPEG, Error) << "Failed to encode stream image"; > return jpeg_size; > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h > index 62c8650..ae636ff 100644 > --- a/src/android/jpeg/post_processor_jpeg.h > +++ b/src/android/jpeg/post_processor_jpeg.h > @@ -23,8 +23,8 @@ public: > > int configure(const libcamera::StreamConfiguration &incfg, > const libcamera::StreamConfiguration &outcfg) override; > - int process(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > + int process(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > CameraMetadata *metadata) override; > > private: > diff --git a/src/android/post_processor.h b/src/android/post_processor.h > index a891c43..5f87a5d 100644 > --- a/src/android/post_processor.h > +++ b/src/android/post_processor.h > @@ -20,8 +20,8 @@ public: > > virtual int configure(const libcamera::StreamConfiguration &inCfg, > const libcamera::StreamConfiguration &outCfg) = 0; > - virtual int process(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > + virtual int process(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > CameraMetadata *metadata) = 0; > }; >
Hi Hiro, On 20/10/2020 11:30, Umang Jain wrote: > Hi Hiro, > > On 10/20/20 1:12 PM, Hirokazu Honda wrote: >> In PostProcessor::process(), the |source| argument doesn't have >> to be a pointer. This replace its type, const pointer, with const s/replace/replaces/ or s/This replace/Replace/ >> reference as the latter is preferred to the former. >> libcamera::Span is cheap 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> > > This looks good to me. > Reviewed-by: Umang Jain <email@uajain.com> >> --- >> src/android/camera_stream.cpp | 2 +- >> src/android/jpeg/post_processor_jpeg.cpp | 6 +++--- >> src/android/jpeg/post_processor_jpeg.h | 4 ++-- >> src/android/post_processor.h | 4 ++-- >> 4 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/src/android/camera_stream.cpp >> b/src/android/camera_stream.cpp >> index 1b8afa8..28a6e09 100644 >> --- a/src/android/camera_stream.cpp >> +++ b/src/android/camera_stream.cpp >> @@ -102,7 +102,7 @@ int CameraStream::process(const >> libcamera::FrameBuffer &source, >> if (!postProcessor_) >> return 0; >> - return postProcessor_->process(&source, dest->maps()[0], >> metadata); >> + return postProcessor_->process(source, dest->maps()[0], metadata); >> } >> FrameBuffer *CameraStream::getBuffer() >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp >> b/src/android/jpeg/post_processor_jpeg.cpp >> index 753c28e..55b9088 100644 >> --- a/src/android/jpeg/post_processor_jpeg.cpp >> +++ b/src/android/jpeg/post_processor_jpeg.cpp >> @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const >> StreamConfiguration &inCfg, >> return encoder_->configure(inCfg); >> } >> -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, >> - const libcamera::Span<uint8_t> &destination, >> +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, >> + libcamera::Span<uint8_t> destination, >> CameraMetadata *metadata) >> { >> if (!encoder_) >> @@ -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()); > I see that this change is required to keep the patch compile-able > because the argument's type has been changed. I would expect this would > be reworked in next patches while correctly the encoder interface. Indeed, I see it in the next patch. This is the right order to do things, as we couldn't change the encoder interface first, as we couldn't give it a reference. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> if (jpeg_size < 0) { >> LOG(JPEG, Error) << "Failed to encode stream image"; >> return jpeg_size; >> diff --git a/src/android/jpeg/post_processor_jpeg.h >> b/src/android/jpeg/post_processor_jpeg.h >> index 62c8650..ae636ff 100644 >> --- a/src/android/jpeg/post_processor_jpeg.h >> +++ b/src/android/jpeg/post_processor_jpeg.h >> @@ -23,8 +23,8 @@ public: >> int configure(const libcamera::StreamConfiguration &incfg, >> const libcamera::StreamConfiguration &outcfg) override; >> - int process(const libcamera::FrameBuffer *source, >> - const libcamera::Span<uint8_t> &destination, >> + int process(const libcamera::FrameBuffer &source, >> + libcamera::Span<uint8_t> destination, >> CameraMetadata *metadata) override; >> private: >> diff --git a/src/android/post_processor.h b/src/android/post_processor.h >> index a891c43..5f87a5d 100644 >> --- a/src/android/post_processor.h >> +++ b/src/android/post_processor.h >> @@ -20,8 +20,8 @@ public: >> virtual int configure(const libcamera::StreamConfiguration >> &inCfg, >> const libcamera::StreamConfiguration &outCfg) = 0; >> - virtual int process(const libcamera::FrameBuffer *source, >> - const libcamera::Span<uint8_t> &destination, >> + virtual int process(const libcamera::FrameBuffer &source, >> + libcamera::Span<uint8_t> destination, >> CameraMetadata *metadata) = 0; >> }; >> > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro-san, Thank you for the patches. For the whole series, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> On Tue, Oct 20, 2020 at 04:42:26PM +0900, Hirokazu Honda wrote: > In PostProcessor::process(), the |source| argument doesn't have > to be a pointer. This replace its type, const pointer, with const > reference as the latter is preferred to the former. > libcamera::Span is cheap 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/camera_stream.cpp | 2 +- > src/android/jpeg/post_processor_jpeg.cpp | 6 +++--- > src/android/jpeg/post_processor_jpeg.h | 4 ++-- > src/android/post_processor.h | 4 ++-- > 4 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index 1b8afa8..28a6e09 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -102,7 +102,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source, > if (!postProcessor_) > return 0; > > - return postProcessor_->process(&source, dest->maps()[0], metadata); > + return postProcessor_->process(source, dest->maps()[0], metadata); > } > > FrameBuffer *CameraStream::getBuffer() > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp > index 753c28e..55b9088 100644 > --- a/src/android/jpeg/post_processor_jpeg.cpp > +++ b/src/android/jpeg/post_processor_jpeg.cpp > @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg, > return encoder_->configure(inCfg); > } > > -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > CameraMetadata *metadata) > { > if (!encoder_) > @@ -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; > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h > index 62c8650..ae636ff 100644 > --- a/src/android/jpeg/post_processor_jpeg.h > +++ b/src/android/jpeg/post_processor_jpeg.h > @@ -23,8 +23,8 @@ public: > > int configure(const libcamera::StreamConfiguration &incfg, > const libcamera::StreamConfiguration &outcfg) override; > - int process(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > + int process(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > CameraMetadata *metadata) override; > > private: > diff --git a/src/android/post_processor.h b/src/android/post_processor.h > index a891c43..5f87a5d 100644 > --- a/src/android/post_processor.h > +++ b/src/android/post_processor.h > @@ -20,8 +20,8 @@ public: > > virtual int configure(const libcamera::StreamConfiguration &inCfg, > const libcamera::StreamConfiguration &outCfg) = 0; > - virtual int process(const libcamera::FrameBuffer *source, > - const libcamera::Span<uint8_t> &destination, > + virtual int process(const libcamera::FrameBuffer &source, > + libcamera::Span<uint8_t> destination, > CameraMetadata *metadata) = 0; > }; >
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index 1b8afa8..28a6e09 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -102,7 +102,7 @@ int CameraStream::process(const libcamera::FrameBuffer &source, if (!postProcessor_) return 0; - return postProcessor_->process(&source, dest->maps()[0], metadata); + return postProcessor_->process(source, dest->maps()[0], metadata); } FrameBuffer *CameraStream::getBuffer() diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp index 753c28e..55b9088 100644 --- a/src/android/jpeg/post_processor_jpeg.cpp +++ b/src/android/jpeg/post_processor_jpeg.cpp @@ -44,8 +44,8 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg, return encoder_->configure(inCfg); } -int PostProcessorJpeg::process(const libcamera::FrameBuffer *source, - const libcamera::Span<uint8_t> &destination, +int PostProcessorJpeg::process(const libcamera::FrameBuffer &source, + libcamera::Span<uint8_t> destination, CameraMetadata *metadata) { if (!encoder_) @@ -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; diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h index 62c8650..ae636ff 100644 --- a/src/android/jpeg/post_processor_jpeg.h +++ b/src/android/jpeg/post_processor_jpeg.h @@ -23,8 +23,8 @@ public: int configure(const libcamera::StreamConfiguration &incfg, const libcamera::StreamConfiguration &outcfg) override; - int process(const libcamera::FrameBuffer *source, - const libcamera::Span<uint8_t> &destination, + int process(const libcamera::FrameBuffer &source, + libcamera::Span<uint8_t> destination, CameraMetadata *metadata) override; private: diff --git a/src/android/post_processor.h b/src/android/post_processor.h index a891c43..5f87a5d 100644 --- a/src/android/post_processor.h +++ b/src/android/post_processor.h @@ -20,8 +20,8 @@ public: virtual int configure(const libcamera::StreamConfiguration &inCfg, const libcamera::StreamConfiguration &outCfg) = 0; - virtual int process(const libcamera::FrameBuffer *source, - const libcamera::Span<uint8_t> &destination, + virtual int process(const libcamera::FrameBuffer &source, + libcamera::Span<uint8_t> destination, CameraMetadata *metadata) = 0; };
In PostProcessor::process(), the |source| argument doesn't have to be a pointer. This replace its type, const pointer, with const reference as the latter is preferred to the former. libcamera::Span is cheap 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/camera_stream.cpp | 2 +- src/android/jpeg/post_processor_jpeg.cpp | 6 +++--- src/android/jpeg/post_processor_jpeg.h | 4 ++-- src/android/post_processor.h | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-)