[libcamera-devel] [PATCH v3 1/3] android: camera_stream: Create post processor in configure()

[Hirokazu Honda]

Hi Laurent,

On Wed, Sep 1, 2021 at 5:33 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Tue, Aug 31, 2021 at 06:34:37PM +0900, Hirokazu Honda wrote:
> > CameraStream creates PostProcessor and FrameBufferAllocator in
> > the constructor. CameraStream assumes that a used post processor
> > is JPEG post processor. Since we need to support various post
> > processors, we would rather move the creation to configure() so
> > as to return an error code if no proper post processor is found.
> > This also moves FrameBufferAllocator and Mutex creation for
> > consistency.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.h   |  1 +
> >  src/android/camera_stream.cpp | 36 ++++++++++++++++++-----------------
> >  2 files changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index a5576927..296c2f18 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -48,6 +48,7 @@ public:
> >
> >       unsigned int id() const { return id_; }
> >       camera3_device_t *camera3Device() { return &camera3Device_; }
> > +     const CameraCapabilities *capabilities() const { return &capabilities_; }
> >       const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; }
> >
> >       const std::string &maker() const { return maker_; }
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 01909ec7..fb10bf06 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -10,6 +10,7 @@
> >  #include <sys/mman.h>
> >
> >  #include "camera_buffer.h"
> > +#include "camera_capabilities.h"
> >  #include "camera_device.h"
> >  #include "camera_metadata.h"
> >  #include "jpeg/post_processor_jpeg.h"
> > @@ -47,20 +48,6 @@ CameraStream::CameraStream(CameraDevice *const cameraDevice,
> >       : cameraDevice_(cameraDevice), config_(config), type_(type),
> >         camera3Stream_(camera3Stream), index_(index)
> >  {
> > -     if (type_ == Type::Internal || type_ == Type::Mapped) {
> > -             /*
> > -              * \todo There might be multiple post-processors. The logic
> > -              * which should be instantiated here, is deferred for the
> > -              * future. For now, we only have PostProcessorJpeg and that
> > -              * is what we instantiate here.
> > -              */
> > -             postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> > -     }
> > -
> > -     if (type == Type::Internal) {
> > -             allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> > -             mutex_ = std::make_unique<std::mutex>();
> > -     }
> >  }
> >
> >  const StreamConfiguration &CameraStream::configuration() const
> > @@ -75,15 +62,30 @@ Stream *CameraStream::stream() const
> >
> >  int CameraStream::configure()
> >  {
> > -     if (postProcessor_) {
> > +     if (type_ == Type::Internal || type_ == Type::Mapped) {
> > +             const PixelFormat outFormat =
> > +                     cameraDevice_->capabilities()->toPixelFormat(camera3Stream_->format);
> >               StreamConfiguration output = configuration();
> > -             output.pixelFormat = formats::MJPEG;
> > +             output.pixelFormat = outFormat;
>
> I'd add a blank line here. Please let me know what you prefer, and I'll
> handle it when pushing.
>

Fair enough.

Thanks,
-Hiro
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +             switch (outFormat) {
> > +             case formats::MJPEG:
> > +                     postProcessor_ = std::make_unique<PostProcessorJpeg>(cameraDevice_);
> > +                     break;
> > +
> > +             default:
> > +                     LOG(HAL, Error) << "Unsupported format: " << outFormat;
> > +                     return -EINVAL;
> > +             }
> > +
> >               int ret = postProcessor_->configure(configuration(), output);
> >               if (ret)
> >                       return ret;
> >       }
> >
> > -     if (allocator_) {
> > +     if (type_ == Type::Internal) {
> > +             allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera());
> > +             mutex_ = std::make_unique<std::mutex>();
> > +
> >               int ret = allocator_->allocate(stream());
> >               if (ret < 0)
> >                       return ret;
>
> --
> Regards,
>
> Laurent Pinchart