[libcamera-devel] [PATCH v2 11/12] android: camera_device: Set Encoder at construction
Hirokazu Honda
hiroh at chromium.org
Tue Sep 8 07:21:20 CEST 2020
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 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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list