[libcamera-devel] [PATCH] android: Modify PostProcessor interface
Hirokazu Honda
hiroh at chromium.org
Mon Oct 19 13:44:24 CEST 2020
Hi Umang,
On Mon, Oct 19, 2020 at 8:24 PM Umang Jain <email at uajain.com> wrote:
>
> Hi Hiro,
>
> On 10/19/20 1:33 PM, Hirokazu Honda wrote:
> > This modifies PostProcessor interface and polishes the code
> > around the PostProcessor.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Changes looks good, albeit a few comments. Also, I think the patch could
> also be split down further to address the per Span pass-by-value, per
> *-type reference to &-type, etc.
Ack. I will do it in the next patch series.
> > ---
> > src/android/camera_stream.cpp | 9 ++++-----
> > src/android/camera_stream.h | 8 ++++----
> > src/android/jpeg/encoder.h | 6 +++---
> > src/android/jpeg/encoder_libjpeg.cpp | 6 +++---
> > src/android/jpeg/encoder_libjpeg.h | 4 ++--
> > src/android/jpeg/post_processor_jpeg.cpp | 8 +++++---
> > src/android/jpeg/post_processor_jpeg.h | 6 +++---
> > src/android/post_processor.h | 4 ++--
> > 8 files changed, 26 insertions(+), 25 deletions(-)
> >
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 1b8afa8..cc8691d 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -40,11 +40,10 @@ LOG_DECLARE_CATEGORY(HAL);
> >
> > CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> > camera3_stream_t *camera3Stream, unsigned int index)
> > - : cameraDevice_(cameraDevice), type_(type),
> > + : cameraDevice_(cameraDevice),
> > + config_(cameraDevice_->cameraConfiguration()), type_(type),
> Will cameraDevice_ will be initialized by this point? I am not sure.
I expect moving here out of body of constructor doesn't make a
difference and thus the answer to your question should be true.
> > camera3Stream_(camera3Stream), index_(index)
> > {
> > - config_ = cameraDevice_->cameraConfiguration();
> > -
> > if (type_ == Type::Internal || type_ == Type::Mapped) {
> > /*
> > * \todo There might be multiple post-processors. The logic
> > @@ -52,7 +51,7 @@ CameraStream::CameraStream(CameraDevice *cameraDevice, Type type,
> > * future. For now, we only have PostProcessorJpeg and that
> > * is what we instantiate here.
> > */
> > - postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> > + postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice);
> What does this change achieve?
Since I change CameraDevice_ to const raw pointer, I need to pass
non-const raw pointer (cameraDevice, not cameraDevice"_") to
PostProcessorJpeg.
We can change PostProcessor and CameraStream constructors to receive
const raw pointer.
> > }
> >
> > if (type == Type::Internal) {
> > @@ -102,7 +101,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/camera_stream.h b/src/android/camera_stream.h
> > index c367a5f..24e66bb 100644
> > --- a/src/android/camera_stream.h
> > +++ b/src/android/camera_stream.h
> > @@ -124,11 +124,11 @@ public:
> > void putBuffer(libcamera::FrameBuffer *buffer);
> >
> > private:
> > - CameraDevice *cameraDevice_;
> > - libcamera::CameraConfiguration *config_;
> > - Type type_;
> > + CameraDevice const *cameraDevice_;
> > + const libcamera::CameraConfiguration *config_;
> > + const Type type_;
> > camera3_stream_t *camera3Stream_;
> > - unsigned int index_;
> > + const unsigned int index_;
> >
> > std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
> > std::vector<libcamera::FrameBuffer *> buffers_;
> > diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> > index 4483153..270ea60 100644
> > --- a/src/android/jpeg/encoder.h
> > +++ b/src/android/jpeg/encoder.h
> > @@ -14,11 +14,11 @@
> > class Encoder
> > {
> > public:
> > - virtual ~Encoder() {};
> > + 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 753c28e..4ded3e3 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_)
> > @@ -64,8 +64,10 @@ int PostProcessorJpeg::process(const libcamera::FrameBuffer *source,
> > * second, it is good enough.
> > */
> > exif.setTimestamp(std::time(nullptr));
> > - if (exif.generate() != 0)
> > + if (exif.generate() != 0) {
> > LOG(JPEG, Error) << "Failed to generate valid EXIF data";
> > + return -EINVAL;
> > + }
> >
> Personally speaking, I don't think we should return FAIL here. So my
> preference is to skip this change.
Thanks. Kieran also told me that in another mail thread.
I will do so in the next patch series.
Regards,
-Hiro
> > int jpeg_size = encoder_->encode(source, destination, exif.data());
> > if (jpeg_size < 0) {
> > diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> > index 62c8650..4d8edf5 100644
> > --- a/src/android/jpeg/post_processor_jpeg.h
> > +++ b/src/android/jpeg/post_processor_jpeg.h
> > @@ -23,12 +23,12 @@ 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:
> > - CameraDevice *cameraDevice_;
> > + CameraDevice const *cameraDevice_;
> > std::unique_ptr<Encoder> encoder_;
> > libcamera::Size streamSize_;
> > };
> > 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;
> > };
> >
>
More information about the libcamera-devel
mailing list