| Message ID | 20190523135900.24029-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On Thu, May 23, 2019 at 02:59:00PM +0100, Kieran Bingham wrote: > UVC pipelines are highly variable, and we can not define their > configuration restrictions within the UVC pipeline handler directly. > > Update the UVCCameraConfiguration to store the UVCCameraData (storing > a reference to the camera as a means of borrowing a reference to the > camera data). > > We then validate the configuration by the tryFormat() operation on the > UVC V4L2Device. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/uvcvideo.cpp | 36 +++++++++++++++++++++-------- > 1 file changed, 26 insertions(+), 10 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index 45260f34c8f5..df321c6e64a6 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -42,9 +42,18 @@ public: > class UVCCameraConfiguration : public CameraConfiguration > { > public: > - UVCCameraConfiguration(); > + UVCCameraConfiguration(Camera *camera, UVCCameraData *data); Minor: do you need 'camera' in UVCCameraConfiguration or just 'data' ? You can pass data to the UVCCameraConfiguration constructor at generateConfiguration() time, and save storing one shared pointer. Apart from this minor thing: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > Status validate() override; > + > +private: > + /* > + * The UVCCameraConfiguration instance is guaranteed to be valid as long > + * as the corresponding Camera instance is valid. In order to borrow a > + * reference to the camera data, store a new reference to the camera. > + */ > + std::shared_ptr<Camera> camera_; > + const UVCCameraData *data_; > }; > > class PipelineHandlerUVC : public PipelineHandler > @@ -76,9 +85,12 @@ private: > } > }; > > -UVCCameraConfiguration::UVCCameraConfiguration() > +UVCCameraConfiguration::UVCCameraConfiguration(Camera *camera, > + UVCCameraData *data) > : CameraConfiguration() > { > + camera_ = camera->shared_from_this(); > + data_ = data; > } > > CameraConfiguration::Status UVCCameraConfiguration::validate() > @@ -96,17 +108,20 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() > > StreamConfiguration &cfg = config_[0]; > > - /* \todo: Validate the configuration against the device capabilities. */ > - const unsigned int pixelFormat = cfg.pixelFormat; > - const Size size = cfg.size; > + V4L2DeviceFormat format; > + format.fourcc = cfg.pixelFormat; > + format.size = cfg.size; > > - cfg.pixelFormat = V4L2_PIX_FMT_YUYV; > - cfg.size = { 640, 480 }; > + /* Validate the format on the device. */ > + data_->video_->tryFormat(&format); > > - if (cfg.pixelFormat != pixelFormat || cfg.size != size) { > + if (cfg.pixelFormat != format.fourcc || cfg.size != format.size) { > LOG(UVC, Debug) > << "Adjusting configuration from " << cfg.toString() > - << " to " << cfg.size.toString() << "-YUYV"; > + << " to " << format.toString(); > + > + cfg.pixelFormat = format.fourcc; > + cfg.size = format.size; > status = Adjusted; > } > > @@ -123,7 +138,8 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager) > CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera, > const StreamRoles &roles) > { > - CameraConfiguration *config = new UVCCameraConfiguration(); > + UVCCameraData *data = cameraData(camera); > + CameraConfiguration *config = new UVCCameraConfiguration(camera, data); > > if (roles.empty()) > return config; > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 24/05/2019 08:56, Jacopo Mondi wrote: > Hi Kieran, > > On Thu, May 23, 2019 at 02:59:00PM +0100, Kieran Bingham wrote: >> UVC pipelines are highly variable, and we can not define their >> configuration restrictions within the UVC pipeline handler directly. >> >> Update the UVCCameraConfiguration to store the UVCCameraData (storing >> a reference to the camera as a means of borrowing a reference to the >> camera data). >> >> We then validate the configuration by the tryFormat() operation on the >> UVC V4L2Device. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/pipeline/uvcvideo.cpp | 36 +++++++++++++++++++++-------- >> 1 file changed, 26 insertions(+), 10 deletions(-) >> >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp >> index 45260f34c8f5..df321c6e64a6 100644 >> --- a/src/libcamera/pipeline/uvcvideo.cpp >> +++ b/src/libcamera/pipeline/uvcvideo.cpp >> @@ -42,9 +42,18 @@ public: >> class UVCCameraConfiguration : public CameraConfiguration >> { >> public: >> - UVCCameraConfiguration(); >> + UVCCameraConfiguration(Camera *camera, UVCCameraData *data); > > Minor: do you need 'camera' in UVCCameraConfiguration or just 'data' ? > You can pass data to the UVCCameraConfiguration constructor at > generateConfiguration() time, and save storing one shared pointer. I added it due to my interpretation of the comment in RkISP1CameraConfiguration (which I have replicated below) as requiring a reference to the Camera to ensure that the CameraData is valid. > Apart from this minor thing: > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thank you, I think Laurent has some reservations about this ... as Niklas is working on some different enumerations. But I still need this series to be able to utilise libcamera on my development laptop, so of course I'd like it to go in ... > > Thanks > j > >> >> Status validate() override; >> + >> +private: >> + /* >> + * The UVCCameraConfiguration instance is guaranteed to be valid as long >> + * as the corresponding Camera instance is valid. In order to borrow a >> + * reference to the camera data, store a new reference to the camera. >> + */ Here: ^^^^^^^^^^^^^^^^^^^ >> + std::shared_ptr<Camera> camera_; >> + const UVCCameraData *data_; >> }; >> >> class PipelineHandlerUVC : public PipelineHandler >> @@ -76,9 +85,12 @@ private: >> } >> }; >> >> -UVCCameraConfiguration::UVCCameraConfiguration() >> +UVCCameraConfiguration::UVCCameraConfiguration(Camera *camera, >> + UVCCameraData *data) >> : CameraConfiguration() >> { >> + camera_ = camera->shared_from_this(); >> + data_ = data; >> } >> >> CameraConfiguration::Status UVCCameraConfiguration::validate() >> @@ -96,17 +108,20 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() >> >> StreamConfiguration &cfg = config_[0]; >> >> - /* \todo: Validate the configuration against the device capabilities. */ >> - const unsigned int pixelFormat = cfg.pixelFormat; >> - const Size size = cfg.size; >> + V4L2DeviceFormat format; >> + format.fourcc = cfg.pixelFormat; >> + format.size = cfg.size; >> >> - cfg.pixelFormat = V4L2_PIX_FMT_YUYV; >> - cfg.size = { 640, 480 }; >> + /* Validate the format on the device. */ >> + data_->video_->tryFormat(&format); >> >> - if (cfg.pixelFormat != pixelFormat || cfg.size != size) { >> + if (cfg.pixelFormat != format.fourcc || cfg.size != format.size) { >> LOG(UVC, Debug) >> << "Adjusting configuration from " << cfg.toString() >> - << " to " << cfg.size.toString() << "-YUYV"; >> + << " to " << format.toString(); >> + >> + cfg.pixelFormat = format.fourcc; >> + cfg.size = format.size; >> status = Adjusted; >> } >> >> @@ -123,7 +138,8 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager) >> CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera, >> const StreamRoles &roles) >> { >> - CameraConfiguration *config = new UVCCameraConfiguration(); >> + UVCCameraData *data = cameraData(camera); >> + CameraConfiguration *config = new UVCCameraConfiguration(camera, data); >> >> if (roles.empty()) >> return config; >> -- >> 2.20.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Fri, May 24, 2019 at 09:25:16AM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 24/05/2019 08:56, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Thu, May 23, 2019 at 02:59:00PM +0100, Kieran Bingham wrote: > >> UVC pipelines are highly variable, and we can not define their > >> configuration restrictions within the UVC pipeline handler directly. > >> > >> Update the UVCCameraConfiguration to store the UVCCameraData (storing > >> a reference to the camera as a means of borrowing a reference to the > >> camera data). > >> > >> We then validate the configuration by the tryFormat() operation on the > >> UVC V4L2Device. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/libcamera/pipeline/uvcvideo.cpp | 36 +++++++++++++++++++++-------- > >> 1 file changed, 26 insertions(+), 10 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > >> index 45260f34c8f5..df321c6e64a6 100644 > >> --- a/src/libcamera/pipeline/uvcvideo.cpp > >> +++ b/src/libcamera/pipeline/uvcvideo.cpp > >> @@ -42,9 +42,18 @@ public: > >> class UVCCameraConfiguration : public CameraConfiguration > >> { > >> public: > >> - UVCCameraConfiguration(); > >> + UVCCameraConfiguration(Camera *camera, UVCCameraData *data); > > > > Minor: do you need 'camera' in UVCCameraConfiguration or just 'data' ? > > You can pass data to the UVCCameraConfiguration constructor at > > generateConfiguration() time, and save storing one shared pointer. > > I added it due to my interpretation of the comment in > RkISP1CameraConfiguration (which I have replicated below) as requiring a > reference to the Camera to ensure that the CameraData is valid. Correct, I've also been told offline just yesterday that this is mostly for reference counting on the Camera instance. Please ignore my comment then! > > > > Apart from this minor thing: > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thank you, > > I think Laurent has some reservations about this ... as Niklas is > working on some different enumerations. > > But I still need this series to be able to utilise libcamera on my > development laptop, so of course I'd like it to go in ... Looking at the code I would have preferred too to see the UVC pipeline handler matching the required configuration against the list of enumerated formats, but since we don't have it yet... :) Anyway, the patch itself is good to me, let's wait to hear from others if we want to wait for format enumeration support to land first. > > > > > > Thanks > > j > > > >> > >> Status validate() override; > >> + > >> +private: > >> + /* > >> + * The UVCCameraConfiguration instance is guaranteed to be valid as long > >> + * as the corresponding Camera instance is valid. In order to borrow a > >> + * reference to the camera data, store a new reference to the camera. > >> + */ > > Here: ^^^^^^^^^^^^^^^^^^^ > > > > >> + std::shared_ptr<Camera> camera_; > >> + const UVCCameraData *data_; > >> }; > >> > >> class PipelineHandlerUVC : public PipelineHandler > >> @@ -76,9 +85,12 @@ private: > >> } > >> }; > >> > >> -UVCCameraConfiguration::UVCCameraConfiguration() > >> +UVCCameraConfiguration::UVCCameraConfiguration(Camera *camera, > >> + UVCCameraData *data) > >> : CameraConfiguration() > >> { > >> + camera_ = camera->shared_from_this(); > >> + data_ = data; > >> } > >> > >> CameraConfiguration::Status UVCCameraConfiguration::validate() > >> @@ -96,17 +108,20 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() > >> > >> StreamConfiguration &cfg = config_[0]; > >> > >> - /* \todo: Validate the configuration against the device capabilities. */ > >> - const unsigned int pixelFormat = cfg.pixelFormat; > >> - const Size size = cfg.size; > >> + V4L2DeviceFormat format; > >> + format.fourcc = cfg.pixelFormat; > >> + format.size = cfg.size; > >> > >> - cfg.pixelFormat = V4L2_PIX_FMT_YUYV; > >> - cfg.size = { 640, 480 }; > >> + /* Validate the format on the device. */ > >> + data_->video_->tryFormat(&format); > >> > >> - if (cfg.pixelFormat != pixelFormat || cfg.size != size) { > >> + if (cfg.pixelFormat != format.fourcc || cfg.size != format.size) { > >> LOG(UVC, Debug) > >> << "Adjusting configuration from " << cfg.toString() > >> - << " to " << cfg.size.toString() << "-YUYV"; > >> + << " to " << format.toString(); > >> + > >> + cfg.pixelFormat = format.fourcc; > >> + cfg.size = format.size; > >> status = Adjusted; > >> } > >> > >> @@ -123,7 +138,8 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager) > >> CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera, > >> const StreamRoles &roles) > >> { > >> - CameraConfiguration *config = new UVCCameraConfiguration(); > >> + UVCCameraData *data = cameraData(camera); > >> + CameraConfiguration *config = new UVCCameraConfiguration(camera, data); > >> > >> if (roles.empty()) > >> return config; > >> -- > >> 2.20.1 > >> > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards > -- > Kieran
diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index 45260f34c8f5..df321c6e64a6 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -42,9 +42,18 @@ public: class UVCCameraConfiguration : public CameraConfiguration { public: - UVCCameraConfiguration(); + UVCCameraConfiguration(Camera *camera, UVCCameraData *data); Status validate() override; + +private: + /* + * The UVCCameraConfiguration instance is guaranteed to be valid as long + * as the corresponding Camera instance is valid. In order to borrow a + * reference to the camera data, store a new reference to the camera. + */ + std::shared_ptr<Camera> camera_; + const UVCCameraData *data_; }; class PipelineHandlerUVC : public PipelineHandler @@ -76,9 +85,12 @@ private: } }; -UVCCameraConfiguration::UVCCameraConfiguration() +UVCCameraConfiguration::UVCCameraConfiguration(Camera *camera, + UVCCameraData *data) : CameraConfiguration() { + camera_ = camera->shared_from_this(); + data_ = data; } CameraConfiguration::Status UVCCameraConfiguration::validate() @@ -96,17 +108,20 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() StreamConfiguration &cfg = config_[0]; - /* \todo: Validate the configuration against the device capabilities. */ - const unsigned int pixelFormat = cfg.pixelFormat; - const Size size = cfg.size; + V4L2DeviceFormat format; + format.fourcc = cfg.pixelFormat; + format.size = cfg.size; - cfg.pixelFormat = V4L2_PIX_FMT_YUYV; - cfg.size = { 640, 480 }; + /* Validate the format on the device. */ + data_->video_->tryFormat(&format); - if (cfg.pixelFormat != pixelFormat || cfg.size != size) { + if (cfg.pixelFormat != format.fourcc || cfg.size != format.size) { LOG(UVC, Debug) << "Adjusting configuration from " << cfg.toString() - << " to " << cfg.size.toString() << "-YUYV"; + << " to " << format.toString(); + + cfg.pixelFormat = format.fourcc; + cfg.size = format.size; status = Adjusted; } @@ -123,7 +138,8 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager) CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera, const StreamRoles &roles) { - CameraConfiguration *config = new UVCCameraConfiguration(); + UVCCameraData *data = cameraData(camera); + CameraConfiguration *config = new UVCCameraConfiguration(camera, data); if (roles.empty()) return config;
UVC pipelines are highly variable, and we can not define their configuration restrictions within the UVC pipeline handler directly. Update the UVCCameraConfiguration to store the UVCCameraData (storing a reference to the camera as a means of borrowing a reference to the camera data). We then validate the configuration by the tryFormat() operation on the UVC V4L2Device. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/uvcvideo.cpp | 36 +++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 10 deletions(-)