[libcamera-devel] [PATCH v2 11/12] android: camera_device: Set Encoder at construction

[Jacopo Mondi]

Hi Hiro, Laurent,

On Tue, Sep 08, 2020 at 09:36:12AM +0300, Laurent Pinchart wrote:
> Hi Hiro-san,
>
> On Tue, Sep 08, 2020 at 02:59:06PM +0900, Hirokazu Honda wrote:
> > On Tue, Sep 8, 2020 at 2:21 PM Hirokazu Honda <hiroh at chromium.org> wrote:
> > > On Mon, Sep 7, 2020 at 5:52 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > >> On Sun, Sep 06, 2020 at 01:09:47AM +0300, Laurent Pinchart wrote:
> > >>> Hi Jacopo,
> > >>>
> > >>> Thank you for the patch.
> > >>>
> > >>> On Wed, Sep 02, 2020 at 05:22:35PM +0200, Jacopo Mondi wrote:
> > >>>> Make the CameraStream Encoder * a private struct member and require its
> > >>>> initialization at construction time.
> > >>>>
> > >>>> This change dis-allow creating a CameraStream and set the Encoder later,
> > >>>
> > >>> s/dis-allow/disallows/
> > >>>
> > >>>> 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.
> > >>>
> > >>> Thinking ahead in the future, do you think this will be a good match
> > >>> when we'll add other post-processing than JPEG encoding (I'm thinking
> > >>> about scaling, rotation and/or format conversion) ? I don't see any
> > >>> particular issue, but a second opinion would be useful.
> > >>
> > >> As discussed with Kieran and briefly with Tomasz, I think we'll have
> > >> to define how to establish which Encoder (or whatever name it will
> > >> be give) interface derived class to instantiate here.
> >
> > I am thinking of introducing PostProcessor interface and regard
> > JpegEncoder as one of frame conversions.
> > I would remove the existing Encoder interface.
> >
> > PostProcessor
> >      +-- FrameConverter (for format conversion)
> >       |       +-- JpegEncoder.
> >       |        |
> >       |       +-- JpegDecoder.
> >       |
> >      +-- FrameScaler     (for frame scaling and cropping)
> >              +-- YUVScaler
> >
> > What do you think?
>
> I agree, I think all post-processing steps should have the same
> interface. I'm not sure if the intermediate FrameConverter and
> FrameScaler classes are needed though. It may be more efficient in some
> cases to perform both scaling and format conversion in one go (for
> instance if we use a JPEG hardware encoder that includes a scaler, or if
> we need to scale and convert from YUV to RGB).

I don't have an idea clear enough to express myself on the layout of
the class hierarchy we'll end up implementing at this time.

But I agree this is the place where we'll need to instantiate the
right Encoder derived class, based on criteria which I like to define
soon.

Proposals are welcome, also based on the experience on how this is
handled in CrOS.

Thanks
  j

>
> > >> I think this part will need to be greatly expanded, and I'm not yet
> > >> sure to which direction (I would likely say a factory of some kind,
> > >> but I think establishing the criteria for selecting which derived
> > >> class to use is more pressing than the implementation itself).
> > >>
> > >> Thanks
> > >>   j
> > >>
> > >>>
> > >>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > >>>
> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >>>
> > >>>> ---
> > >>>>  src/android/camera_device.cpp | 23 ++++++++++++-----------
> > >>>>  src/android/camera_device.h   |  7 ++++---
> > >>>>  2 files changed, 16 insertions(+), 14 deletions(-)
> > >>>>
> > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > >>>> index 9bcd1d993c17..28d8e081eab0 100644
> > >>>> --- a/src/android/camera_device.cpp
> > >>>> +++ b/src/android/camera_device.cpp
> > >>>> @@ -168,14 +168,14 @@ 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)
> > >>>> +   : format(f), size(s), index_(i), encoder_(encoder)
> > >>>>  {
> > >>>>  }
> > >>>>
> > >>>>  CameraStream::~CameraStream()
> > >>>>  {
> > >>>> -   delete jpeg;
> > >>>> +   delete encoder_;
> > >>>>  };
> > >>>>
> > >>>>  /*
> > >>>> @@ -1279,20 +1279,21 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >>>>             PixelFormat format = formats::MJPEG;
> > >>>>             Size size = cfg.size;
> > >>>>
> > >>>> -           CameraStream &cameraStream = streams_.emplace_back(format, size, index);
> > >>>> -           stream->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";
> > >>>> +                   LOG(HAL, Error) << "Failed to configure encoder";
> > >>>> +                   delete encoder;
> > >>>>                     return ret;
> > >>>>             }
> > >>>> +
> > >>>> +           CameraStream &cameraStream = streams_.emplace_back(format, size,
> > >>>> +                                                              index, encoder);
> > >>>> +           stream->priv = static_cast<void *>(&cameraStream);
> > >>>>     }
> > >>>>
> > >>>>     switch (config_->validate()) {
> > >>>> @@ -1481,7 +1482,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..3c57ffec265d 100644
> > >>>> --- a/src/android/camera_device.h
> > >>>> +++ b/src/android/camera_device.h
> > >>>> @@ -29,16 +29,16 @@ class CameraMetadata;
> > >>>>
> > >>>>  struct CameraStream {
> > >>>>  public:
> > >>>> -   CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i);
> > >>>> +   CameraStream(libcamera::PixelFormat, libcamera::Size, unsigned int i,
> > >>>> +                Encoder *encoder = nullptr);
> > >>>>     ~CameraStream();
> > >>>>
> > >>>>     unsigned int index() const { return index_; }
> > >>>> +   Encoder *encoder() const { return encoder_; }
> > >>>>
> > >>>>     libcamera::PixelFormat format;
> > >>>>     libcamera::Size size;
> > >>>>
> > >>>> -   Encoder *jpeg;
> > >>>> -
> > >>>>  private:
> > >>>>     /*
> > >>>>      * The index of the libcamera StreamConfiguration as added during
> > >>>> @@ -46,6 +46,7 @@ private:
> > >>>>      * one or more streams to the Android framework.
> > >>>>      */
> > >>>>     unsigned int index_;
> > >>>> +   Encoder *encoder_;
> > >>>>  };
> > >>>>
> > >>>>  class CameraDevice : protected libcamera::Loggable
> > >>>
> > >
> > > I would like to manage encoder_ with std::unique_ptr<> in CameraStream.
>
> --
> Regards,
>
> Laurent Pinchart