[libcamera-devel,v3,10/11] android: camera_device: Set Encoder at construction
diff mbox series

Message ID 20200908134142.27470-11-jacopo@jmondi.org
State Accepted
Headers show
Series
  • android: camera_device: Fix JPEG/RAW sizes
Related show

Commit Message

Jacopo Mondi Sept. 8, 2020, 1:41 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 22 +++++++++-------------
 src/android/camera_device.h   |  8 ++++----
 2 files changed, 13 insertions(+), 17 deletions(-)

Comments

Niklas Söderlund Sept. 10, 2020, 11:06 a.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.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)

I would either name the argument 'e' or expand the other arguments to 
their full names mixing does not look nice.

> +	: 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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Sept. 10, 2020, 11:15 a.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.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)
>
> 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@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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
Hirokazu Honda Sept. 11, 2020, 2:40 a.m. UTC | #3 
On Thu, Sep 10, 2020 at 8:12 PM Jacopo Mondi <jacopo@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@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>


Reviewed-by: Hirokazu Honda <hiroh@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)
> >
> > 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@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@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Hirokazu Honda Sept. 11, 2020, 2:42 a.m. UTC | #4 
On Fri, Sep 11, 2020 at 11:40 AM Hirokazu Honda <hiroh@chromium.org> wrote:
>
> On Thu, Sep 10, 2020 at 8:12 PM Jacopo Mondi <jacopo@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@ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>
>
> Reviewed-by: Hirokazu Honda <hiroh@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@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@lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Sept. 11, 2020, 4:32 p.m. UTC | #5 
Hi Hiro,

On Fri, Sep 11, 2020 at 11:42:34AM +0900, Hirokazu Honda wrote:
> On Fri, Sep 11, 2020 at 11:40 AM Hirokazu Honda <hiroh@chromium.org> wrote:
> >
> > On Thu, Sep 10, 2020 at 8:12 PM Jacopo Mondi <jacopo@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@ideasonboard.com>
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> >
> > Reviewed-by: Hirokazu Honda <hiroh@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>/
>

But then I would have to move() at construction.. Is it worth it ?
I'll check the usage pattern and see how it looks like.

Thanks
  j

>
> > > > 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@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@lists.libcamera.org
> > > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > > >
> > > > --
> > > > Regards,
> > > > Niklas Söderlund
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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)
+	: 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());
 	}
 
 	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