[libcamera-devel] [PATCH v3 10/11] android: camera_device: Set Encoder at construction

[Hirokazu Honda]

On Fri, Sep 11, 2020 at 11:40 AM Hirokazu Honda <hiroh at chromium.org> wrote:
>
> On Thu, Sep 10, 2020 at 8:12 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi Niklas,
> >
> > On Thu, Sep 10, 2020 at 01:06:05PM +0200, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your work.
> > >
> > > On 2020-09-08 15:41:41 +0200, Jacopo Mondi wrote:
> > > > Make the CameraStream encoder a private unique pointer and require its
> > > > initialization at construction time. This ties the encoder lifetime to
> > > > the CameraStream it has been created with, allowing to remove the
> > > > CameraStream destructor.
> > > >
> > > > This change dis-allow creating a CameraStream and set the Encoder later,
> > > > which shall not happen now that we create CameraStream once we have all
> > > > the required information in place.
> > > >
> > > > No functional changes intended but this change aims to make the code
> > > > more robust enforcing a stricter CameraStream interface.
> > > >
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>
>
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > > > ---
> > > >  src/android/camera_device.cpp | 22 +++++++++-------------
> > > >  src/android/camera_device.h   |  8 ++++----
> > > >  2 files changed, 13 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index e0260c92246c..4c1416913d00 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -168,16 +168,11 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
> > > >     }
> > > >  }
> > > >
> > > > -CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i)
> > > > -   : format(f), size(s), jpeg(nullptr), index_(i)
> > > > +CameraStream::CameraStream(PixelFormat f, Size s, unsigned int i, Encoder *encoder)
> > >

To make sure we get the ownership,
s/Encoder */std::unique_ptr<Encoder>/


> > > I would either name the argument 'e' or expand the other arguments to
> > > their full names mixing does not look nice.
> >
> > Arguments names are expanded in the last patch of the series.
> >
> > >
> > > > +   : format(f), size(s), index_(i), encoder_(encoder)
> > > >  {
> > > >  }
> > > >
> > > > -CameraStream::~CameraStream()
> > > > -{
> > > > -   delete jpeg;
> > > > -};
> > > > -
> > > >  /*
> > > >   * \struct Camera3RequestDescriptor
> > > >   *
> > > > @@ -1271,20 +1266,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > >             }
> > > >
> > > >             StreamConfiguration &cfg = config_->at(index);
> > > > -           CameraStream &cameraStream =
> > > > -                   streams_.emplace_back(formats::MJPEG, cfg.size, index);
> > > > -           jpegStream->priv = static_cast<void *>(&cameraStream);
> > > >
> > > >             /*
> > > >              * Construct a software encoder for MJPEG streams from the
> > > >              * chosen libcamera source stream.
> > > >              */
> > > > -           cameraStream.jpeg = new EncoderLibJpeg();
> > > > -           int ret = cameraStream.jpeg->configure(cfg);
> > > > +           Encoder *encoder = new EncoderLibJpeg();
> > > > +           int ret = encoder->configure(cfg);
> > > >             if (ret) {
> > > >                     LOG(HAL, Error) << "Failed to configure encoder";
> > > > +                   delete encoder;
> > > >                     return ret;
> > > >             }
> > > > +
> > > > +           streams_.emplace_back(formats::MJPEG, cfg.size, index, encoder);
> > > > +           jpegStream->priv = static_cast<void *>(&streams_.back());
> > >
> > > Same comment as in previous patch about pointer to vector member. As
> > > that should be sorted in that patch and I do like this change, with the
> > > arguments comment addressed above,
> > >
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > >
> > > >     }
> > > >
> > > >     switch (config_->validate()) {
> > > > @@ -1473,7 +1469,7 @@ void CameraDevice::requestComplete(Request *request)
> > > >             if (cameraStream->format != formats::MJPEG)
> > > >                     continue;
> > > >
> > > > -           Encoder *encoder = cameraStream->jpeg;
> > > > +           Encoder *encoder = cameraStream->encoder();
> > > >             if (!encoder) {
> > > >                     LOG(HAL, Error) << "Failed to identify encoder";
> > > >                     continue;
> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > index f8f237203ce9..5ac8f05e5a07 100644
> > > > --- a/src/android/camera_device.h
> > > > +++ b/src/android/camera_device.h
> > > > @@ -29,16 +29,15 @@ class CameraMetadata;
> > > >
> > > >  struct CameraStream {
> > > >  public:
> > > > -   CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
> > > > -   ~CameraStream();
> > > > +   CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,
> > > > +                Encoder *encoder = nullptr);
> > > >
> > > >     unsigned int index() const { return index_; }
> > > > +   Encoder *encoder() const { return encoder_.get(); }
> > > >
> > > >     libcamera::PixelFormat format;
> > > >     libcamera::Size size;
> > > >
> > > > -   Encoder *jpeg;
> > > > -
> > > >  private:
> > > >     /*
> > > >      * The index of the libcamera StreamConfiguration as added during
> > > > @@ -46,6 +45,7 @@ private:
> > > >      * one or more streams to the Android framework.
> > > >      */
> > > >     unsigned int index_;
> > > > +   std::unique_ptr<Encoder> encoder_;
> > > >  };
> > > >
> > > >  class CameraDevice : protected libcamera::Loggable
> > > > --
> > > > 2.28.0
> > > >
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel at lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel