| Message ID | 20200521135416.13685-5-email@uajain.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Thu, May 21, 2020 at 01:54:26PM +0000, Umang Jain wrote: > Hook up various QCam UI bits with hotplug support introduced > in previous commits. This looks good-enough as first steps > to see how the hotplugging functionality is turning out to be > from application point-of-view. > > One can still think of few edge case nuances not yet covered > under this implementation hence, those are intentionally > kept out of scope for now. It might require some thinking > and/or additional time on hand. Out of curiosity, do you have any such cases in mind already ? > Signed-off-by: Umang Jain <email@uajain.com> > --- > src/qcam/main_window.cpp | 83 ++++++++++++++++++++++++++++++++++++++++ > src/qcam/main_window.h | 6 +++ > 2 files changed, 89 insertions(+) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 7de0895..ba96c27 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -49,6 +49,43 @@ public: > } > }; > > +/** > + * \brief Custom QEvent to signal hotplug or unplug > + */ > +class HotplugEvent : public QEvent > +{ > +public: > + enum PLUGEVENT { We Use CamelCase for types, this would be PlugEvent. > + HOTPLUG, > + UNPLUG Same here, Hotplug, Unplug, Kieran will hate me for asking if it should be Plug, Unplug, , Hotplug, Hotunplug, or HotPlug, HotUnplug, My personal preference is (from most preferred to least preferred) HotUnplug, Unplug, Hotunplug, but at the same time I wouldn't rename HotplugEvent to HotPlugEvent. Maybe we could use "hotplug" (Hotplug in CamelCase) as a generic term that would cover both plug and unplug, and "hot-plug" and "hot-unplug" (respectively HotPlug and HotUnplug in CamelCase) when talking about plug and unplug specifically ? Or maybe that's overkill :-) > + }; > + > + HotplugEvent(std::shared_ptr<Camera> camera, PLUGEVENT event) > + : QEvent(type()) : QEvent(type()), camera_(std::move(camera)), plugEvent_(event) { } > + { > + camera_ = camera; > + plugEvent_ = event; > + } > + > + ~HotplugEvent() > + { > + camera_.reset(); This isn't needed, the shared pointer is automatically reset when destroyed. You can drop the HotplugEvent destructor. > + } > + > + static Type type() > + { > + static int type = QEvent::registerEventType(); > + return static_cast<Type>(type); > + } > + > + PLUGEVENT getHotplugEvent() { return plugEvent_; } We don't usually prefix getters with get, PlugEvent hotplugEvent() const { return plugEvent; } (with an added const) > + Camera *getCamera() { return camera_.get(); } Same here. > + > +private: > + std::shared_ptr<Camera> camera_; > + PLUGEVENT plugEvent_; > +}; > + > MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) > : saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr), > isCapturing_(false), captureRaw_(false) > @@ -71,6 +108,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) > setCentralWidget(viewfinder_); > adjustSize(); > > + /* Hotplug/unplug support */ > + cm_->newCameraAdded.connect(this, &MainWindow::addNewCamera); > + cm_->cameraRemoved.connect(this, &MainWindow::removeCamera); > + > /* Open the camera and start capture. */ > ret = openCamera(); > if (ret < 0) { > @@ -95,6 +136,9 @@ bool MainWindow::event(QEvent *e) > if (e->type() == CaptureEvent::type()) { > processCapture(); > return true; > + } else if (e->type() == HotplugEvent::type()) { > + processHotplug(static_cast<HotplugEvent *>(e)); > + return true; > } > > return QMainWindow::event(e); > @@ -525,6 +569,45 @@ void MainWindow::stopCapture() > setWindowTitle(title_); > } > > +/* ----------------------------------------------------------------------------- > + * Camera hotplugging support > + */ > + > +void MainWindow::processHotplug(HotplugEvent *e) > +{ > + Camera *camera = e->getCamera(); > + HotplugEvent::PLUGEVENT event = e->getHotplugEvent(); > + > + if (event == HotplugEvent::PLUGEVENT::HOTPLUG) { This can be shortened to HotplugEvent::HOTPLUG (same for similar occurences below). > + cameraCombo_->addItem(QString::fromStdString(camera->name())); > + } else if (event == HotplugEvent::PLUGEVENT::UNPLUG) { > + int camIndex = cameraCombo_->findText(QString::fromStdString(camera->name())); I'd move this line right before cameraCombo_->removeItem() (with a blank line above) to keep the variable declaration close to its usage. > + > + /* Check if the currently-streaming camera is removed. */ > + if (camera == camera_.get()) { > + toggleCapture(false); > + cameraCombo_->setCurrentIndex(0); > + } > + cameraCombo_->removeItem(camIndex); > + } > +} > + > +void MainWindow::addNewCamera(std::shared_ptr<Camera> camera) If we rename CameraManager::newCameraAdded this should be renamed to addCamera(). That's quite a few comments, but nothing major. I believe the next version will be the last. Thanks for your work ! > +{ > + qInfo() << "Adding new camera:" << camera->name().c_str(); > + QCoreApplication::postEvent(this, > + new HotplugEvent(std::move(camera), > + HotplugEvent::PLUGEVENT::HOTPLUG)); > +} > + > +void MainWindow::removeCamera(std::shared_ptr<Camera> camera) > +{ > + qInfo() << "Removing camera:" << camera->name().c_str(); > + QCoreApplication::postEvent(this, > + new HotplugEvent(std::move(camera), > + HotplugEvent::PLUGEVENT::UNPLUG)); > +} > + > /* ----------------------------------------------------------------------------- > * Image Save > */ > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > index 59fa2d9..9108780 100644 > --- a/src/qcam/main_window.h > +++ b/src/qcam/main_window.h > @@ -32,6 +32,8 @@ using namespace libcamera; > class QAction; > class QComboBox; > > +class HotplugEvent; > + > enum { > OptCamera = 'c', > OptHelp = 'h', > @@ -87,8 +89,12 @@ private: > int startCapture(); > void stopCapture(); > > + void addNewCamera(std::shared_ptr<Camera> camera); > + void removeCamera(std::shared_ptr<Camera> camera); > + > void requestComplete(Request *request); > void processCapture(); > + void processHotplug(HotplugEvent *e); > void processViewfinder(FrameBuffer *buffer); > > /* UI elements */
Hi Laurent, On 6/7/20 10:34 PM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Thu, May 21, 2020 at 01:54:26PM +0000, Umang Jain wrote: >> Hook up various QCam UI bits with hotplug support introduced >> in previous commits. This looks good-enough as first steps >> to see how the hotplugging functionality is turning out to be >> from application point-of-view. >> >> One can still think of few edge case nuances not yet covered >> under this implementation hence, those are intentionally >> kept out of scope for now. It might require some thinking >> and/or additional time on hand. > Out of curiosity, do you have any such cases in mind already ? Yes, They were mostly around having one camera on the system. Hot-unplugging the only camera present and plugging it back in, does not start streaming it out of the box. Might need to handle this case a bit differently in qcam, which honestly I was hoping to address after these patch series is landed, in chunks of small improvements. > >> Signed-off-by: Umang Jain <email@uajain.com> >> --- >> src/qcam/main_window.cpp | 83 ++++++++++++++++++++++++++++++++++++++++ >> src/qcam/main_window.h | 6 +++ >> 2 files changed, 89 insertions(+) >> >> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp >> index 7de0895..ba96c27 100644 >> --- a/src/qcam/main_window.cpp >> +++ b/src/qcam/main_window.cpp >> @@ -49,6 +49,43 @@ public: >> } >> }; >> >> +/** >> + * \brief Custom QEvent to signal hotplug or unplug >> + */ >> +class HotplugEvent : public QEvent >> +{ >> +public: >> + enum PLUGEVENT { > We Use CamelCase for types, this would be PlugEvent. > >> + HOTPLUG, >> + UNPLUG > Same here, > > Hotplug, > Unplug, > > Kieran will hate me for asking if it should be > > Plug, > Unplug, > > , > > Hotplug, > Hotunplug, > > or > > HotPlug, > HotUnplug, > > My personal preference is (from most preferred to least preferred) > HotUnplug, Unplug, Hotunplug, but at the same time I wouldn't rename > HotplugEvent to HotPlugEvent. Maybe we could use "hotplug" (Hotplug in > CamelCase) as a generic term that would cover both plug and unplug, and > "hot-plug" and "hot-unplug" (respectively HotPlug and HotUnplug in > CamelCase) when talking about plug and unplug specifically ? Or maybe > that's overkill :-) > >> + }; >> + >> + HotplugEvent(std::shared_ptr<Camera> camera, PLUGEVENT event) >> + : QEvent(type()) > : QEvent(type()), camera_(std::move(camera)), plugEvent_(event) > { > } > >> + { >> + camera_ = camera; >> + plugEvent_ = event; >> + } >> + >> + ~HotplugEvent() >> + { >> + camera_.reset(); > This isn't needed, the shared pointer is automatically reset when > destroyed. You can drop the HotplugEvent destructor. > >> + } >> + >> + static Type type() >> + { >> + static int type = QEvent::registerEventType(); >> + return static_cast<Type>(type); >> + } >> + >> + PLUGEVENT getHotplugEvent() { return plugEvent_; } > We don't usually prefix getters with get, > > PlugEvent hotplugEvent() const { return plugEvent; } > > (with an added const) > >> + Camera *getCamera() { return camera_.get(); } > Same here. > >> + >> +private: >> + std::shared_ptr<Camera> camera_; >> + PLUGEVENT plugEvent_; >> +}; >> + >> MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) >> : saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr), >> isCapturing_(false), captureRaw_(false) >> @@ -71,6 +108,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) >> setCentralWidget(viewfinder_); >> adjustSize(); >> >> + /* Hotplug/unplug support */ >> + cm_->newCameraAdded.connect(this, &MainWindow::addNewCamera); >> + cm_->cameraRemoved.connect(this, &MainWindow::removeCamera); >> + >> /* Open the camera and start capture. */ >> ret = openCamera(); >> if (ret < 0) { >> @@ -95,6 +136,9 @@ bool MainWindow::event(QEvent *e) >> if (e->type() == CaptureEvent::type()) { >> processCapture(); >> return true; >> + } else if (e->type() == HotplugEvent::type()) { >> + processHotplug(static_cast<HotplugEvent *>(e)); >> + return true; >> } >> >> return QMainWindow::event(e); >> @@ -525,6 +569,45 @@ void MainWindow::stopCapture() >> setWindowTitle(title_); >> } >> >> +/* ----------------------------------------------------------------------------- >> + * Camera hotplugging support >> + */ >> + >> +void MainWindow::processHotplug(HotplugEvent *e) >> +{ >> + Camera *camera = e->getCamera(); >> + HotplugEvent::PLUGEVENT event = e->getHotplugEvent(); >> + >> + if (event == HotplugEvent::PLUGEVENT::HOTPLUG) { > This can be shortened to HotplugEvent::HOTPLUG (same for similar > occurences below). > >> + cameraCombo_->addItem(QString::fromStdString(camera->name())); >> + } else if (event == HotplugEvent::PLUGEVENT::UNPLUG) { >> + int camIndex = cameraCombo_->findText(QString::fromStdString(camera->name())); > I'd move this line right before cameraCombo_->removeItem() (with a blank > line above) to keep the variable declaration close to its usage. > >> + >> + /* Check if the currently-streaming camera is removed. */ >> + if (camera == camera_.get()) { >> + toggleCapture(false); >> + cameraCombo_->setCurrentIndex(0); >> + } >> + cameraCombo_->removeItem(camIndex); >> + } >> +} >> + >> +void MainWindow::addNewCamera(std::shared_ptr<Camera> camera) > If we rename CameraManager::newCameraAdded this should be renamed to > addCamera(). > > That's quite a few comments, but nothing major. I believe the next > version will be the last. Thanks for your work ! > >> +{ >> + qInfo() << "Adding new camera:" << camera->name().c_str(); >> + QCoreApplication::postEvent(this, >> + new HotplugEvent(std::move(camera), >> + HotplugEvent::PLUGEVENT::HOTPLUG)); >> +} >> + >> +void MainWindow::removeCamera(std::shared_ptr<Camera> camera) >> +{ >> + qInfo() << "Removing camera:" << camera->name().c_str(); >> + QCoreApplication::postEvent(this, >> + new HotplugEvent(std::move(camera), >> + HotplugEvent::PLUGEVENT::UNPLUG)); >> +} >> + >> /* ----------------------------------------------------------------------------- >> * Image Save >> */ >> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h >> index 59fa2d9..9108780 100644 >> --- a/src/qcam/main_window.h >> +++ b/src/qcam/main_window.h >> @@ -32,6 +32,8 @@ using namespace libcamera; >> class QAction; >> class QComboBox; >> >> +class HotplugEvent; >> + >> enum { >> OptCamera = 'c', >> OptHelp = 'h', >> @@ -87,8 +89,12 @@ private: >> int startCapture(); >> void stopCapture(); >> >> + void addNewCamera(std::shared_ptr<Camera> camera); >> + void removeCamera(std::shared_ptr<Camera> camera); >> + >> void requestComplete(Request *request); >> void processCapture(); >> + void processHotplug(HotplugEvent *e); >> void processViewfinder(FrameBuffer *buffer); >> >> /* UI elements */
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index 7de0895..ba96c27 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -49,6 +49,43 @@ public: } }; +/** + * \brief Custom QEvent to signal hotplug or unplug + */ +class HotplugEvent : public QEvent +{ +public: + enum PLUGEVENT { + HOTPLUG, + UNPLUG + }; + + HotplugEvent(std::shared_ptr<Camera> camera, PLUGEVENT event) + : QEvent(type()) + { + camera_ = camera; + plugEvent_ = event; + } + + ~HotplugEvent() + { + camera_.reset(); + } + + static Type type() + { + static int type = QEvent::registerEventType(); + return static_cast<Type>(type); + } + + PLUGEVENT getHotplugEvent() { return plugEvent_; } + Camera *getCamera() { return camera_.get(); } + +private: + std::shared_ptr<Camera> camera_; + PLUGEVENT plugEvent_; +}; + MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) : saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr), isCapturing_(false), captureRaw_(false) @@ -71,6 +108,10 @@ MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) setCentralWidget(viewfinder_); adjustSize(); + /* Hotplug/unplug support */ + cm_->newCameraAdded.connect(this, &MainWindow::addNewCamera); + cm_->cameraRemoved.connect(this, &MainWindow::removeCamera); + /* Open the camera and start capture. */ ret = openCamera(); if (ret < 0) { @@ -95,6 +136,9 @@ bool MainWindow::event(QEvent *e) if (e->type() == CaptureEvent::type()) { processCapture(); return true; + } else if (e->type() == HotplugEvent::type()) { + processHotplug(static_cast<HotplugEvent *>(e)); + return true; } return QMainWindow::event(e); @@ -525,6 +569,45 @@ void MainWindow::stopCapture() setWindowTitle(title_); } +/* ----------------------------------------------------------------------------- + * Camera hotplugging support + */ + +void MainWindow::processHotplug(HotplugEvent *e) +{ + Camera *camera = e->getCamera(); + HotplugEvent::PLUGEVENT event = e->getHotplugEvent(); + + if (event == HotplugEvent::PLUGEVENT::HOTPLUG) { + cameraCombo_->addItem(QString::fromStdString(camera->name())); + } else if (event == HotplugEvent::PLUGEVENT::UNPLUG) { + int camIndex = cameraCombo_->findText(QString::fromStdString(camera->name())); + + /* Check if the currently-streaming camera is removed. */ + if (camera == camera_.get()) { + toggleCapture(false); + cameraCombo_->setCurrentIndex(0); + } + cameraCombo_->removeItem(camIndex); + } +} + +void MainWindow::addNewCamera(std::shared_ptr<Camera> camera) +{ + qInfo() << "Adding new camera:" << camera->name().c_str(); + QCoreApplication::postEvent(this, + new HotplugEvent(std::move(camera), + HotplugEvent::PLUGEVENT::HOTPLUG)); +} + +void MainWindow::removeCamera(std::shared_ptr<Camera> camera) +{ + qInfo() << "Removing camera:" << camera->name().c_str(); + QCoreApplication::postEvent(this, + new HotplugEvent(std::move(camera), + HotplugEvent::PLUGEVENT::UNPLUG)); +} + /* ----------------------------------------------------------------------------- * Image Save */ diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h index 59fa2d9..9108780 100644 --- a/src/qcam/main_window.h +++ b/src/qcam/main_window.h @@ -32,6 +32,8 @@ using namespace libcamera; class QAction; class QComboBox; +class HotplugEvent; + enum { OptCamera = 'c', OptHelp = 'h', @@ -87,8 +89,12 @@ private: int startCapture(); void stopCapture(); + void addNewCamera(std::shared_ptr<Camera> camera); + void removeCamera(std::shared_ptr<Camera> camera); + void requestComplete(Request *request); void processCapture(); + void processHotplug(HotplugEvent *e); void processViewfinder(FrameBuffer *buffer); /* UI elements */
Hook up various QCam UI bits with hotplug support introduced in previous commits. This looks good-enough as first steps to see how the hotplugging functionality is turning out to be from application point-of-view. One can still think of few edge case nuances not yet covered under this implementation hence, those are intentionally kept out of scope for now. It might require some thinking and/or additional time on hand. Signed-off-by: Umang Jain <email@uajain.com> --- src/qcam/main_window.cpp | 83 ++++++++++++++++++++++++++++++++++++++++ src/qcam/main_window.h | 6 +++ 2 files changed, 89 insertions(+)