[libcamera-devel] [PATCH 19/30] cam: Move camera acquire to the CameraSession class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 12 20:40:07 CEST 2021
Hi Kieran,
On Mon, Jul 12, 2021 at 04:43:51PM +0100, Kieran Bingham wrote:
> On 07/07/2021 03:19, Laurent Pinchart wrote:
> > Continue moving towards making the CameraSession class the central point
> > to handle a camera by moving the camera acquire operation. A new
> > CameraSession::camera() function is needed to allow access to the camera
> > from CamApp.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/cam/camera_session.cpp | 29 ++++++++++++++++++++--
> > src/cam/camera_session.h | 6 ++++-
> > src/cam/main.cpp | 49 +++++++++-----------------------------
> > 3 files changed, 43 insertions(+), 41 deletions(-)
> >
> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > index edc7205d6bda..10e66446ee67 100644
> > --- a/src/cam/camera_session.cpp
> > +++ b/src/cam/camera_session.cpp
> > @@ -19,11 +19,30 @@
> >
> > using namespace libcamera;
> >
> > -CameraSession::CameraSession(std::shared_ptr<Camera> camera,
> > +CameraSession::CameraSession(CameraManager *cm,
> > const OptionsParser::Options &options)
> > - : camera_(camera), last_(0), queueCount_(0), captureCount_(0),
> > + : last_(0), queueCount_(0), captureCount_(0),
> > captureLimit_(0), printMetadata_(false)
>
> Well I certainly like seeing initialiser lists shrink ;-)
>
> > {
> > + const std::string &cameraId = options[OptCamera];
> > + char *endptr;
> > + unsigned long index = strtoul(cameraId.c_str(), &endptr, 10);
> > + if (*endptr == '\0' && index > 0 && index <= cm->cameras().size())
> > + camera_ = cm->cameras()[index - 1];
> > + else
> > + camera_ = cm->get(cameraId);
> > +
> > + if (!camera_) {
> > + std::cerr << "Camera " << cameraId << " not found" << std::endl;
> > + return;
> > + }
> > +
> > + if (camera_->acquire()) {
> > + std::cerr << "Failed to acquire camera " << cameraId
> > + << std::endl;
> > + return;
> > + }
> > +
> > StreamRoles roles = StreamKeyValueParser::roles(options[OptStream]);
> >
> > std::unique_ptr<CameraConfiguration> config =
> > @@ -64,6 +83,12 @@ CameraSession::CameraSession(std::shared_ptr<Camera> camera,
> > config_ = std::move(config);
> > }
> >
> > +CameraSession::~CameraSession()
> > +{
> > + if (camera_)
> > + camera_->release();
>
> Makes me wonder if we should we wrap a shared_ptr<camera> with a
> unique_ptr so that it automatically releases ?
>
> And I can't quite tell where the sarcasm stops or the ... why we might
> need that starts.... eeek
:-)
Jokes aside, I've been thinking about replacing the
std::shared_ptr<Camera> with an application-allocated
std::unique_ptr<Camera>. This Camera would be a facade object that would
wrap an std::shared_ptr<Camera> internally (we would need two different
names for Camera). I'm not sure at this point if it's worth it.
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > +}
> > +
> > int CameraSession::start(const OptionsParser::Options &options)
> > {
> > int ret;
> > diff --git a/src/cam/camera_session.h b/src/cam/camera_session.h
> > index 39dbbdf37913..88baf9061629 100644
> > --- a/src/cam/camera_session.h
> > +++ b/src/cam/camera_session.h
> > @@ -14,6 +14,7 @@
> > #include <libcamera/base/signal.h>
> >
> > #include <libcamera/camera.h>
> > +#include <libcamera/camera_manager.h>
> > #include <libcamera/framebuffer.h>
> > #include <libcamera/framebuffer_allocator.h>
> > #include <libcamera/request.h>
> > @@ -25,10 +26,13 @@
> > class CameraSession
> > {
> > public:
> > - CameraSession(std::shared_ptr<libcamera::Camera> camera,
> > + CameraSession(libcamera::CameraManager *cm,
> > const OptionsParser::Options &options);
> > + ~CameraSession();
> >
> > bool isValid() const { return config_ != nullptr; }
> > +
> > + libcamera::Camera *camera() { return camera_.get(); }
> > libcamera::CameraConfiguration *config() { return config_.get(); }
> >
> > int start(const OptionsParser::Options &options);
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index 082a053efeda..8ed5e841a1e9 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -51,7 +51,6 @@ private:
> > OptionsParser::Options options_;
> > CameraManager *cm_;
> >
> > - std::shared_ptr<Camera> camera_;
> > std::unique_ptr<CameraSession> session_;
> >
> > EventLoop loop_;
> > @@ -60,7 +59,7 @@ private:
> > CamApp *CamApp::app_ = nullptr;
> >
> > CamApp::CamApp()
> > - : cm_(nullptr), camera_(nullptr)
> > + : cm_(nullptr)
> > {
> > CamApp::app_ = this;
> > }
> > @@ -93,37 +92,16 @@ int CamApp::init(int argc, char **argv)
> > }
> >
> > if (options_.isSet(OptCamera)) {
> > - const std::string &cameraId = options_[OptCamera];
> > - char *endptr;
> > - unsigned long index = strtoul(cameraId.c_str(), &endptr, 10);
> > - if (*endptr == '\0' && index > 0 && index <= cm_->cameras().size())
> > - camera_ = cm_->cameras()[index - 1];
> > - else
> > - camera_ = cm_->get(cameraId);
> > -
> > - if (!camera_) {
> > - std::cout << "Camera "
> > - << std::string(options_[OptCamera])
> > - << " not found" << std::endl;
> > - cleanup();
> > - return -ENODEV;
> > - }
> > -
> > - if (camera_->acquire()) {
> > - std::cout << "Failed to acquire camera" << std::endl;
> > - cleanup();
> > - return -EINVAL;
> > - }
> > -
> > - std::cout << "Using camera " << camera_->id() << std::endl;
> > -
> > - session_ = std::make_unique<CameraSession>(camera_, options_);
> > + session_ = std::make_unique<CameraSession>(cm_, options_);
> > if (!session_->isValid()) {
> > std::cout << "Failed to create camera session" << std::endl;
> > cleanup();
> > return -EINVAL;
> > }
> >
> > + std::cout << "Using camera " << session_->camera()->id()
> > + << std::endl;
> > +
> > session_->captureDone.connect(this, &CamApp::captureDone);
> > }
> >
> > @@ -138,11 +116,6 @@ int CamApp::init(int argc, char **argv)
> >
> > void CamApp::cleanup()
> > {
> > - if (camera_) {
> > - camera_->release();
> > - camera_.reset();
> > - }
> > -
> > session_.reset();
> >
> > cm_->stop();
> > @@ -214,13 +187,13 @@ int CamApp::parseOptions(int argc, char *argv[])
> >
> > int CamApp::listControls()
> > {
> > - if (!camera_) {
> > + if (!session_) {
> > std::cout << "Cannot list controls without a camera"
> > << std::endl;
> > return -EINVAL;
> > }
> >
> > - for (const auto &ctrl : camera_->controls()) {
> > + for (const auto &ctrl : session_->camera()->controls()) {
> > const ControlId *id = ctrl.first;
> > const ControlInfo &info = ctrl.second;
> >
> > @@ -233,13 +206,13 @@ int CamApp::listControls()
> >
> > int CamApp::listProperties()
> > {
> > - if (!camera_) {
> > + if (!session_) {
> > std::cout << "Cannot list properties without a camera"
> > << std::endl;
> > return -EINVAL;
> > }
> >
> > - for (const auto &prop : camera_->properties()) {
> > + for (const auto &prop : session_->camera()->properties()) {
> > const ControlId *id = properties::properties.at(prop.first);
> > const ControlValue &value = prop.second;
> >
> > @@ -252,7 +225,7 @@ int CamApp::listProperties()
> >
> > int CamApp::infoConfiguration()
> > {
> > - if (!camera_) {
> > + if (!session_) {
> > std::cout << "Cannot print stream information without a camera"
> > << std::endl;
> > return -EINVAL;
> > @@ -328,7 +301,7 @@ int CamApp::run()
> > }
> >
> > if (options_.isSet(OptCapture)) {
> > - if (!camera_) {
> > + if (!session_) {
> > std::cout << "Can't capture without a camera" << std::endl;
> > return -ENODEV;
> > }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list