[libcamera-devel] [PATCH 09/10] cam: Store camera as shared pointer everywhere

[Niklas Söderlund]

Hi Jacopo, Laurent,

Thanks for your feedback.

On 2019-11-18 13:41:54 +0200, Laurent Pinchart wrote:
> Hello,
> 
> On Wed, Nov 06, 2019 at 09:13:40AM +0100, Jacopo Mondi wrote:
> > On Mon, Oct 28, 2019 at 03:22:23AM +0100, Niklas Söderlund wrote:
> > > Do not store the camera raw pointer in the capture class, this will
> > > prevent forwarding the shared pointer in the future.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  src/cam/capture.cpp | 2 +-
> > >  src/cam/capture.h   | 4 ++--
> > >  src/cam/main.cpp    | 2 +-
> > >  3 files changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > > index 27332df1d55a5c2d..e665d819fb777a90 100644
> > > --- a/src/cam/capture.cpp
> > > +++ b/src/cam/capture.cpp
> > > @@ -16,7 +16,7 @@
> > >
> > >  using namespace libcamera;
> > >
> > > -Capture::Capture(Camera *camera, CameraConfiguration *config)
> > > +Capture::Capture(std::shared_ptr<Camera> camera, CameraConfiguration *config)
> > >  	: camera_(camera), config_(config), writer_(nullptr)
> > >  {
> > >  }
> > > diff --git a/src/cam/capture.h b/src/cam/capture.h
> > > index 4d396afb8c771a74..c692d48918f2de1d 100644
> > > --- a/src/cam/capture.h
> > > +++ b/src/cam/capture.h
> > > @@ -21,7 +21,7 @@
> > >  class Capture
> > >  {
> > >  public:
> > > -	Capture(libcamera::Camera *camera,
> > > +	Capture(std::shared_ptr<libcamera::Camera> camera,
> > >  		libcamera::CameraConfiguration *config);
> > >
> > >  	int run(EventLoop *loop, const OptionsParser::Options &options);
> > > @@ -30,7 +30,7 @@ private:
> > >
> > >  	void requestComplete(libcamera::Request *request);
> > >
> > > -	libcamera::Camera *camera_;
> > > +	std::shared_ptr<libcamera::Camera> camera_;
> > >  	libcamera::CameraConfiguration *config_;
> > >
> > >  	std::map<libcamera::Stream *, std::string> streamName_;
> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > > index 9d99f5587cbb77d0..a38cca959aca05ff 100644
> > > --- a/src/cam/main.cpp
> > > +++ b/src/cam/main.cpp
> > > @@ -319,7 +319,7 @@ int CamApp::run()
> > >  	}
> > >
> > >  	if (options_.isSet(OptCapture)) {
> > > -		Capture capture(camera_.get(), config_.get());
> > > +		Capture capture(camera_, config_.get());
> > >  		return capture.run(loop_, options_);
> > 
> > I'm not sure increasing the reference count of the shared pointer to
> > store it in an object whose lifetime is limited to this two lines is
> > worth. It doesn't hurt for sure, so I'm fine if you want to keep it.
> 
> I share Jacopo's opinion. I haven't checked if this change is needed for
> your buffer rework. If it's not, I'm not opposed to it, but I'm also not
> sure it's really worth it.

It's needed for the buffer rework so it could be moved to that series.  
But out of taste I also think this is the right thing to do so I kept it 
in the cleanup series.

I will collect Jacopo's and Kieran's tags for this and send it out in v2 
but let me know if you think it should be moved to the buffer api 
series.

> 
> > >  	}
> > >
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund