[libcamera-devel] [PATCH] android: Modify PostProcessor interface

[Tomasz Figa]

On Mon, Oct 19, 2020 at 1: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.

+1

I'd also see rationale explained for each change.

> > ---
> >   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.
> >         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?
> >       }
> >
> >       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.

I'd consider this to be governed by Android expectations.

Hiro, did you base on some kind of documented expectations when making
this change? If not, would you mind researching what's required for
compliance here? Thanks!

Best regards,
Tomasz