[libcamera-devel] [PATCH v2 13/13] libcamera: ipu3: imgu: Use unique_ptr for video and subdevices

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sun, Jun 28, 2020 at 02:15:32AM +0200, Niklas Söderlund wrote:
> Instead of manually deleting the video and subdevices in the destructor
> use std::unique_ptr.
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp | 24 ++++++++++++++-------
>  src/libcamera/pipeline/ipu3/imgu.h   | 32 ++++++++--------------------
>  2 files changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 6a721d47cdacf3d6..4f11dc0dbb5fe94a 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -44,28 +44,36 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  	 * by the match() function: no need to check for newly created
>  	 * video devices and subdevice validity here.
>  	 */
> -	imgu_ = V4L2Subdevice::fromEntityName(media, name_);
> +	imgu_ = std::unique_ptr<V4L2Subdevice>(
> +			V4L2Subdevice::fromEntityName(media, name_));

How about

	imgu_.reset(V4L2Subdevice::fromEntityName(media, name_));

and similarly below ?

I'm considering a patch on top of this to make
V4L2Subdevice::fromEntityName() return a unique_ptr.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  	ret = imgu_->open();
>  	if (ret)
>  		return ret;
>  
> -	input_ = V4L2VideoDevice::fromEntityName(media, name_ + " input");
> +	input_ = std::unique_ptr<V4L2VideoDevice>(
> +			V4L2VideoDevice::fromEntityName(media,
> +							name_ + " input"));
>  	ret = input_->open();
>  	if (ret)
>  		return ret;
>  
> -	output_ = V4L2VideoDevice::fromEntityName(media, name_ + " output");
> +	output_ = std::unique_ptr<V4L2VideoDevice>(
> +			V4L2VideoDevice::fromEntityName(media,
> +							name_ + " output"));
>  	ret = output_->open();
>  	if (ret)
>  		return ret;
>  
> -	viewfinder_ = V4L2VideoDevice::fromEntityName(media,
> -						      name_ + " viewfinder");
> +	viewfinder_ = std::unique_ptr<V4L2VideoDevice>(
> +			V4L2VideoDevice::fromEntityName(media,
> +							name_ + " viewfinder"));
>  	ret = viewfinder_->open();
>  	if (ret)
>  		return ret;
>  
> -	stat_ = V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat");
> +	stat_ = std::unique_ptr<V4L2VideoDevice>(
> +			V4L2VideoDevice::fromEntityName(media,
> +							name_ + " 3a stat"));
>  	ret = stat_->open();
>  	if (ret)
>  		return ret;
> @@ -150,7 +158,7 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
>  		return ret;
>  
>  	/* No need to apply format to the stat node. */
> -	if (dev == stat_)
> +	if (dev == stat_.get())
>  		return 0;
>  
>  	*outputFormat = {};
> @@ -162,7 +170,7 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
>  	if (ret)
>  		return ret;
>  
> -	const char *name = dev == output_ ? "output" : "viewfinder";
> +	const char *name = dev == output_.get() ? "output" : "viewfinder";
>  	LOG(IPU3, Debug) << "ImgU " << name << " format = "
>  			 << outputFormat->toString();
>  
> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> index 7be25e40957fcb03..308bf26ee56e4e82 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.h
> +++ b/src/libcamera/pipeline/ipu3/imgu.h
> @@ -24,21 +24,6 @@ struct StreamConfiguration;
>  class ImgUDevice
>  {
>  public:
> -	ImgUDevice()
> -		: imgu_(nullptr), input_(nullptr), output_(nullptr),
> -		  viewfinder_(nullptr), stat_(nullptr)
> -	{
> -	}
> -
> -	~ImgUDevice()
> -	{
> -		delete imgu_;
> -		delete input_;
> -		delete output_;
> -		delete viewfinder_;
> -		delete stat_;
> -	}
> -
>  	int init(MediaDevice *media, unsigned int index);
>  
>  	int configureInput(const Size &size, V4L2DeviceFormat *inputFormat);
> @@ -46,21 +31,22 @@ public:
>  	int configureOutput(const StreamConfiguration &cfg,
>  			    V4L2DeviceFormat *outputFormat)
>  	{
> -		return configureVideoDevice(output_, PAD_OUTPUT, cfg,
> +		return configureVideoDevice(output_.get(), PAD_OUTPUT, cfg,
>  					    outputFormat);
>  	}
>  
>  	int configureViewfinder(const StreamConfiguration &cfg,
>  				V4L2DeviceFormat *outputFormat)
>  	{
> -		return configureVideoDevice(viewfinder_, PAD_VF, cfg,
> +		return configureVideoDevice(viewfinder_.get(), PAD_VF, cfg,
>  					    outputFormat);
>  	}
>  
>  	int configureStat(const StreamConfiguration &cfg,
>  			  V4L2DeviceFormat *outputFormat)
>  	{
> -		return configureVideoDevice(stat_, PAD_STAT, cfg, outputFormat);
> +		return configureVideoDevice(stat_.get(), PAD_STAT, cfg,
> +					    outputFormat);
>  	}
>  
>  	int allocateBuffers(unsigned int bufferCount);
> @@ -71,11 +57,11 @@ public:
>  
>  	int enableLinks(bool enable);
>  
> -	V4L2Subdevice *imgu_;
> -	V4L2VideoDevice *input_;
> -	V4L2VideoDevice *output_;
> -	V4L2VideoDevice *viewfinder_;
> -	V4L2VideoDevice *stat_;
> +	std::unique_ptr<V4L2Subdevice> imgu_;
> +	std::unique_ptr<V4L2VideoDevice> input_;
> +	std::unique_ptr<V4L2VideoDevice> output_;
> +	std::unique_ptr<V4L2VideoDevice> viewfinder_;
> +	std::unique_ptr<V4L2VideoDevice> stat_;
>  	/* \todo Add param video device for 3A tuning */
>  
>  private:

-- 
Regards,

Laurent Pinchart