| Message ID | 20191028022224.795355-10-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, thanks for the patch 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@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. > } > > -- > 2.23.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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. > > } > >
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@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
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_); }
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@ragnatech.se> --- src/cam/capture.cpp | 2 +- src/cam/capture.h | 4 ++-- src/cam/main.cpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)