| Message ID | 20190228162913.6508-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your patch. On 2019-02-28 18:29:05 +0200, Laurent Pinchart wrote: > Subclassing CameraData will become mandatory for pipeline handlers. > Create a new UVCCameraData class and instantiate it when creating > cameras to prepare for that change. The video_ and stream_ fields of the > UVC pipeline handler belong to the camera data, move them there. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/uvcvideo.cpp | 74 +++++++++++++++++++++-------- > 1 file changed, 53 insertions(+), 21 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index dd20bb085a29..4ad311163a95 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -13,6 +13,7 @@ > #include "log.h" > #include "media_device.h" > #include "pipeline_handler.h" > +#include "utils.h" > #include "v4l2_device.h" > > namespace libcamera { > @@ -42,21 +43,38 @@ public: > bool match(DeviceEnumerator *enumerator); > > private: > + class UVCCameraData : public CameraData > + { > + public: > + UVCCameraData() > + { > + } > + > + ~UVCCameraData() > + { > + delete video_; > + } > + > + V4L2Device *video_; > + Stream stream_; > + }; > + > + UVCCameraData *cameraData(const Camera *camera) > + { > + return static_cast<UVCCameraData *>( > + PipelineHandler::cameraData(camera)); > + } > + > std::shared_ptr<MediaDevice> media_; > - V4L2Device *video_; > - Stream stream_; > }; > > PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager) > - : PipelineHandler(manager), media_(nullptr), video_(nullptr) > + : PipelineHandler(manager), media_(nullptr) > { > } > > PipelineHandlerUVC::~PipelineHandlerUVC() > { > - if (video_) > - delete video_; > - > if (media_) > media_->release(); > } > @@ -65,8 +83,9 @@ std::map<Stream *, StreamConfiguration> > PipelineHandlerUVC::streamConfiguration(Camera *camera, > std::vector<Stream *> &streams) > { > + UVCCameraData *data = cameraData(camera); > + > std::map<Stream *, StreamConfiguration> configs; > - > StreamConfiguration config{}; > > LOG(UVC, Debug) << "Retrieving default format"; > @@ -75,7 +94,7 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera, > config.pixelFormat = V4L2_PIX_FMT_YUYV; > config.bufferCount = 4; > > - configs[&stream_] = config; > + configs[&data->stream_] = config; > > return configs; > } > @@ -83,7 +102,8 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera, > int PipelineHandlerUVC::configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) > { > - StreamConfiguration *cfg = &config[&stream_]; > + UVCCameraData *data = cameraData(camera); > + StreamConfiguration *cfg = &config[&data->stream_]; > int ret; > > LOG(UVC, Debug) << "Configure the camera for resolution " > @@ -94,7 +114,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > format.height = cfg->height; > format.fourcc = cfg->pixelFormat; > > - ret = video_->setFormat(&format); > + ret = data->video_->setFormat(&format); > if (ret) > return ret; > > @@ -108,31 +128,36 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > > int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > { > + UVCCameraData *data = cameraData(camera); > const StreamConfiguration &cfg = stream->configuration(); > > LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > > - return video_->exportBuffers(&stream->bufferPool()); > + return data->video_->exportBuffers(&stream->bufferPool()); > } > > int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) > { > - return video_->releaseBuffers(); > + UVCCameraData *data = cameraData(camera); > + return data->video_->releaseBuffers(); > } > > int PipelineHandlerUVC::start(const Camera *camera) > { > - return video_->streamOn(); > + UVCCameraData *data = cameraData(camera); > + return data->video_->streamOn(); > } > > void PipelineHandlerUVC::stop(const Camera *camera) > { > - video_->streamOff(); > + UVCCameraData *data = cameraData(camera); > + data->video_->streamOff(); > } > > int PipelineHandlerUVC::queueRequest(const Camera *camera, Request *request) > { > - Buffer *buffer = request->findBuffer(&stream_); > + UVCCameraData *data = cameraData(camera); > + Buffer *buffer = request->findBuffer(&data->stream_); > if (!buffer) { > LOG(UVC, Error) > << "Attempt to queue request with invalid stream"; > @@ -140,7 +165,7 @@ int PipelineHandlerUVC::queueRequest(const Camera *camera, Request *request) > return -ENOENT; > } > > - video_->queueBuffer(buffer); > + data->video_->queueBuffer(buffer); > > return 0; > } > @@ -150,29 +175,36 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > DeviceMatch dm("uvcvideo"); > > media_ = enumerator->search(dm); > - > if (!media_) > return false; > > media_->acquire(); > > + std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(); > + > + /* Locate and open the default video node. */ > for (MediaEntity *entity : media_->entities()) { > if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { > - video_ = new V4L2Device(entity); > + data->video_ = new V4L2Device(entity); > break; > } > } > > - if (!video_ || video_->open()) { > - if (!video_) > + if (!data->video_ || data->video_->open()) { > + if (!data->video_) > LOG(UVC, Error) << "Could not find a default video device"; > > return false; > } > > - std::vector<Stream *> streams{ &stream_ }; > + /* Create and register the camera. */ > + std::vector<Stream *> streams{ &data->stream_ }; > std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams); > + > + setCameraData(camera.get(), std::move(data)); > registerCamera(std::move(camera)); > + > + /* Enable hot-unplug notifications. */ > hotplugMediaDevice(media_.get()); > > return true; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Thu, Feb 28, 2019 at 06:29:05PM +0200, Laurent Pinchart wrote: > Subclassing CameraData will become mandatory for pipeline handlers. > Create a new UVCCameraData class and instantiate it when creating > cameras to prepare for that change. The video_ and stream_ fields of the > UVC pipeline handler belong to the camera data, move them there. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/uvcvideo.cpp | 74 +++++++++++++++++++++-------- > 1 file changed, 53 insertions(+), 21 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index dd20bb085a29..4ad311163a95 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -13,6 +13,7 @@ > #include "log.h" > #include "media_device.h" > #include "pipeline_handler.h" > +#include "utils.h" > #include "v4l2_device.h" > > namespace libcamera { > @@ -42,21 +43,38 @@ public: > bool match(DeviceEnumerator *enumerator); > > private: > + class UVCCameraData : public CameraData > + { > + public: > + UVCCameraData() nit: : video_(nullptr) > + { > + } > + > + ~UVCCameraData() > + { > + delete video_; > + } > + > + V4L2Device *video_; > + Stream stream_; > + }; > + > + UVCCameraData *cameraData(const Camera *camera) > + { > + return static_cast<UVCCameraData *>( > + PipelineHandler::cameraData(camera)); > + } > + > std::shared_ptr<MediaDevice> media_; > - V4L2Device *video_; > - Stream stream_; > }; > > PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager) > - : PipelineHandler(manager), media_(nullptr), video_(nullptr) > + : PipelineHandler(manager), media_(nullptr) > { > } > > PipelineHandlerUVC::~PipelineHandlerUVC() > { > - if (video_) > - delete video_; > - > if (media_) > media_->release(); > } > @@ -65,8 +83,9 @@ std::map<Stream *, StreamConfiguration> > PipelineHandlerUVC::streamConfiguration(Camera *camera, > std::vector<Stream *> &streams) > { > + UVCCameraData *data = cameraData(camera); > + > std::map<Stream *, StreamConfiguration> configs; > - > StreamConfiguration config{}; > > LOG(UVC, Debug) << "Retrieving default format"; > @@ -75,7 +94,7 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera, > config.pixelFormat = V4L2_PIX_FMT_YUYV; > config.bufferCount = 4; > > - configs[&stream_] = config; > + configs[&data->stream_] = config; > > return configs; > } > @@ -83,7 +102,8 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera, > int PipelineHandlerUVC::configureStreams(Camera *camera, > std::map<Stream *, StreamConfiguration> &config) > { > - StreamConfiguration *cfg = &config[&stream_]; > + UVCCameraData *data = cameraData(camera); > + StreamConfiguration *cfg = &config[&data->stream_]; > int ret; > > LOG(UVC, Debug) << "Configure the camera for resolution " > @@ -94,7 +114,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > format.height = cfg->height; > format.fourcc = cfg->pixelFormat; > > - ret = video_->setFormat(&format); > + ret = data->video_->setFormat(&format); > if (ret) > return ret; > > @@ -108,31 +128,36 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, > > int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > { > + UVCCameraData *data = cameraData(camera); > const StreamConfiguration &cfg = stream->configuration(); > > LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > > - return video_->exportBuffers(&stream->bufferPool()); > + return data->video_->exportBuffers(&stream->bufferPool()); > } > > int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) > { > - return video_->releaseBuffers(); > + UVCCameraData *data = cameraData(camera); > + return data->video_->releaseBuffers(); > } > > int PipelineHandlerUVC::start(const Camera *camera) > { > - return video_->streamOn(); > + UVCCameraData *data = cameraData(camera); > + return data->video_->streamOn(); > } > > void PipelineHandlerUVC::stop(const Camera *camera) > { > - video_->streamOff(); > + UVCCameraData *data = cameraData(camera); > + data->video_->streamOff(); > } > > int PipelineHandlerUVC::queueRequest(const Camera *camera, Request *request) > { > - Buffer *buffer = request->findBuffer(&stream_); > + UVCCameraData *data = cameraData(camera); > + Buffer *buffer = request->findBuffer(&data->stream_); > if (!buffer) { > LOG(UVC, Error) > << "Attempt to queue request with invalid stream"; > @@ -140,7 +165,7 @@ int PipelineHandlerUVC::queueRequest(const Camera *camera, Request *request) > return -ENOENT; > } > > - video_->queueBuffer(buffer); > + data->video_->queueBuffer(buffer); > > return 0; > } > @@ -150,29 +175,36 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > DeviceMatch dm("uvcvideo"); > > media_ = enumerator->search(dm); > - > if (!media_) > return false; > > media_->acquire(); > > + std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(); > + > + /* Locate and open the default video node. */ > for (MediaEntity *entity : media_->entities()) { > if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { > - video_ = new V4L2Device(entity); > + data->video_ = new V4L2Device(entity); > break; > } > } > > - if (!video_ || video_->open()) { > - if (!video_) > + if (!data->video_ || data->video_->open()) { > + if (!data->video_) This double (!data->video_) check is weird. Could you check for !data->video_ after the for loop, and handle the open() return code here? This check would also possibly benefit from the video_(nullptr) initialization in UVCCameraData constructor. > LOG(UVC, Error) << "Could not find a default video device"; > > return false; > } > > - std::vector<Stream *> streams{ &stream_ }; > + /* Create and register the camera. */ > + std::vector<Stream *> streams{ &data->stream_ }; > std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams); > + > + setCameraData(camera.get(), std::move(data)); > registerCamera(std::move(camera)); > + > + /* Enable hot-unplug notifications. */ > hotplugMediaDevice(media_.get()); > > return true; > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index dd20bb085a29..4ad311163a95 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -13,6 +13,7 @@ #include "log.h" #include "media_device.h" #include "pipeline_handler.h" +#include "utils.h" #include "v4l2_device.h" namespace libcamera { @@ -42,21 +43,38 @@ public: bool match(DeviceEnumerator *enumerator); private: + class UVCCameraData : public CameraData + { + public: + UVCCameraData() + { + } + + ~UVCCameraData() + { + delete video_; + } + + V4L2Device *video_; + Stream stream_; + }; + + UVCCameraData *cameraData(const Camera *camera) + { + return static_cast<UVCCameraData *>( + PipelineHandler::cameraData(camera)); + } + std::shared_ptr<MediaDevice> media_; - V4L2Device *video_; - Stream stream_; }; PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager) - : PipelineHandler(manager), media_(nullptr), video_(nullptr) + : PipelineHandler(manager), media_(nullptr) { } PipelineHandlerUVC::~PipelineHandlerUVC() { - if (video_) - delete video_; - if (media_) media_->release(); } @@ -65,8 +83,9 @@ std::map<Stream *, StreamConfiguration> PipelineHandlerUVC::streamConfiguration(Camera *camera, std::vector<Stream *> &streams) { + UVCCameraData *data = cameraData(camera); + std::map<Stream *, StreamConfiguration> configs; - StreamConfiguration config{}; LOG(UVC, Debug) << "Retrieving default format"; @@ -75,7 +94,7 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera, config.pixelFormat = V4L2_PIX_FMT_YUYV; config.bufferCount = 4; - configs[&stream_] = config; + configs[&data->stream_] = config; return configs; } @@ -83,7 +102,8 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera, int PipelineHandlerUVC::configureStreams(Camera *camera, std::map<Stream *, StreamConfiguration> &config) { - StreamConfiguration *cfg = &config[&stream_]; + UVCCameraData *data = cameraData(camera); + StreamConfiguration *cfg = &config[&data->stream_]; int ret; LOG(UVC, Debug) << "Configure the camera for resolution " @@ -94,7 +114,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, format.height = cfg->height; format.fourcc = cfg->pixelFormat; - ret = video_->setFormat(&format); + ret = data->video_->setFormat(&format); if (ret) return ret; @@ -108,31 +128,36 @@ int PipelineHandlerUVC::configureStreams(Camera *camera, int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) { + UVCCameraData *data = cameraData(camera); const StreamConfiguration &cfg = stream->configuration(); LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; - return video_->exportBuffers(&stream->bufferPool()); + return data->video_->exportBuffers(&stream->bufferPool()); } int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) { - return video_->releaseBuffers(); + UVCCameraData *data = cameraData(camera); + return data->video_->releaseBuffers(); } int PipelineHandlerUVC::start(const Camera *camera) { - return video_->streamOn(); + UVCCameraData *data = cameraData(camera); + return data->video_->streamOn(); } void PipelineHandlerUVC::stop(const Camera *camera) { - video_->streamOff(); + UVCCameraData *data = cameraData(camera); + data->video_->streamOff(); } int PipelineHandlerUVC::queueRequest(const Camera *camera, Request *request) { - Buffer *buffer = request->findBuffer(&stream_); + UVCCameraData *data = cameraData(camera); + Buffer *buffer = request->findBuffer(&data->stream_); if (!buffer) { LOG(UVC, Error) << "Attempt to queue request with invalid stream"; @@ -140,7 +165,7 @@ int PipelineHandlerUVC::queueRequest(const Camera *camera, Request *request) return -ENOENT; } - video_->queueBuffer(buffer); + data->video_->queueBuffer(buffer); return 0; } @@ -150,29 +175,36 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) DeviceMatch dm("uvcvideo"); media_ = enumerator->search(dm); - if (!media_) return false; media_->acquire(); + std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(); + + /* Locate and open the default video node. */ for (MediaEntity *entity : media_->entities()) { if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { - video_ = new V4L2Device(entity); + data->video_ = new V4L2Device(entity); break; } } - if (!video_ || video_->open()) { - if (!video_) + if (!data->video_ || data->video_->open()) { + if (!data->video_) LOG(UVC, Error) << "Could not find a default video device"; return false; } - std::vector<Stream *> streams{ &stream_ }; + /* Create and register the camera. */ + std::vector<Stream *> streams{ &data->stream_ }; std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams); + + setCameraData(camera.get(), std::move(data)); registerCamera(std::move(camera)); + + /* Enable hot-unplug notifications. */ hotplugMediaDevice(media_.get()); return true;
Subclassing CameraData will become mandatory for pipeline handlers. Create a new UVCCameraData class and instantiate it when creating cameras to prepare for that change. The video_ and stream_ fields of the UVC pipeline handler belong to the camera data, move them there. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/uvcvideo.cpp | 74 +++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 21 deletions(-)