| Message ID | 20190207212119.30299-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran,
ignorant question, is FL_DEFAULT an UVC specific thing?
A quick grep on kernel sources returns:
$ git grep FL_DEFAULT drivers/media/
drivers/media/platform/omap3isp/ispresizer.c: res->video_out.video.entity.flags |= MEDIA_ENT_FL_DEFAULT;
drivers/media/usb/uvc/uvc_entity.c: entity->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
So I guess not, as omap3 seems also omap3 uses that as well.
If that's not a UVC specificity, I welcome this change.
On Thu, Feb 07, 2019 at 09:21:15PM +0000, Kieran Bingham wrote:
> Add a helper to identify the default entity from a media device.
>
> Utilise it in the two places which iterate the entities manually, to allocated
> a V4L2Device from the default entity.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> src/libcamera/include/media_device.h | 1 +
> src/libcamera/media_device.cpp | 15 +++++++++++++++
> src/libcamera/pipeline/uvcvideo.cpp | 10 ++++------
> test/v4l2_device/v4l2_device_test.cpp | 12 +++++-------
> 4 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index 9f038093b2b2..361e8f4a4b86 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -42,6 +42,7 @@ public:
>
> const std::vector<MediaEntity *> &entities() const { return entities_; }
> MediaEntity *getEntityByName(const std::string &name) const;
> + MediaEntity *defaultEntity() const;
>
> MediaLink *link(const std::string &sourceName, unsigned int sourceIdx,
> const std::string &sinkName, unsigned int sinkIdx);
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 800ed330fe6d..4af90e1590a1 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -319,6 +319,21 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const
> return nullptr;
> }
>
> +/**
> + * \brief Return the default MediaEntity within a MediaDevice
> + * \return The default entity if specified, or a nullptr otherwise
> + */
> +MediaEntity *MediaDevice::defaultEntity() const
> +{
> + for (MediaEntity *entity : entities_) {
> + if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> + return entity;
> + }
You could drop the inner braces { } here
> + }
> +
> + return nullptr;
> +}
> +
> /**
> * \brief Retrieve the MediaLink connecting two pads, identified by entity
> * names and pad indexes
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index fc31c52c0ecd..ed8228bb2fc6 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -146,13 +146,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>
> media_->acquire();
>
> - for (MediaEntity *entity : media_->entities()) {
> - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> - video_ = new V4L2Device(entity);
> - break;
> - }
> - }
> + MediaEntity *entity = media_->defaultEntity();
> + if (!entity)
Looking at existing code if you don't find any DEFAULT entity, you
return false as you do here, but also release the media device.
Should the same be done here?
> + return false;
>
> + video_ = new V4L2Device(entity);
> if (!video_ || video_->open()) {
> if (!video_)
> LOG(UVC, Error) << "Could not find a default video device";
> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
> index 18d014caf4c8..dd28cccada6b 100644
> --- a/test/v4l2_device/v4l2_device_test.cpp
> +++ b/test/v4l2_device/v4l2_device_test.cpp
> @@ -46,15 +46,13 @@ int V4L2DeviceTest::init()
>
> media_->acquire();
>
> - for (MediaEntity *entity : media_->entities()) {
> - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> - dev_ = new V4L2Device(entity);
> - break;
> - }
> - }
> + MediaEntity *entity = media_->defaultEntity();
> + if (!entity)
> + return TestFail;
>
> + dev_ = new V4L2Device(entity);
> if (!dev_)
> - return TestSkip;
> + return TestFail;
>
> return dev_->open();
> }
> --
> 2.19.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Thu, Feb 07, 2019 at 11:06:20PM +0100, Jacopo Mondi wrote: > Hi Kieran, > ignorant question, is FL_DEFAULT an UVC specific thing? It isn't, but it isn't widely used either. It would thus be fine using the default entity in UVC-specific tests or in the UVC pipeline handler, but not in the rest of the code, at least not without a fallback. > A quick grep on kernel sources returns: > > $ git grep FL_DEFAULT drivers/media/ > drivers/media/platform/omap3isp/ispresizer.c: res->video_out.video.entity.flags |= MEDIA_ENT_FL_DEFAULT; > drivers/media/usb/uvc/uvc_entity.c: entity->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT; > > So I guess not, as omap3 seems also omap3 uses that as well. > > If that's not a UVC specificity, I welcome this change. > > On Thu, Feb 07, 2019 at 09:21:15PM +0000, Kieran Bingham wrote: > > Add a helper to identify the default entity from a media device. > > > > Utilise it in the two places which iterate the entities manually, to allocated > > a V4L2Device from the default entity. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/libcamera/include/media_device.h | 1 + > > src/libcamera/media_device.cpp | 15 +++++++++++++++ > > src/libcamera/pipeline/uvcvideo.cpp | 10 ++++------ > > test/v4l2_device/v4l2_device_test.cpp | 12 +++++------- > > 4 files changed, 25 insertions(+), 13 deletions(-) > > > > diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h > > index 9f038093b2b2..361e8f4a4b86 100644 > > --- a/src/libcamera/include/media_device.h > > +++ b/src/libcamera/include/media_device.h > > @@ -42,6 +42,7 @@ public: > > > > const std::vector<MediaEntity *> &entities() const { return entities_; } > > MediaEntity *getEntityByName(const std::string &name) const; > > + MediaEntity *defaultEntity() const; This should take an entity type as a parameter as the default flag is defined as "Default entity for its type". It could also be used, for instance, to discover the default camera sensor. > > > > MediaLink *link(const std::string &sourceName, unsigned int sourceIdx, > > const std::string &sinkName, unsigned int sinkIdx); > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > > index 800ed330fe6d..4af90e1590a1 100644 > > --- a/src/libcamera/media_device.cpp > > +++ b/src/libcamera/media_device.cpp > > @@ -319,6 +319,21 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const > > return nullptr; > > } > > > > +/** > > + * \brief Return the default MediaEntity within a MediaDevice > > + * \return The default entity if specified, or a nullptr otherwise > > + */ > > +MediaEntity *MediaDevice::defaultEntity() const > > +{ > > + for (MediaEntity *entity : entities_) { > > + if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { > > + return entity; > > + } > > You could drop the inner braces { } here > > + } > > + > > + return nullptr; > > +} > > + > > /** > > * \brief Retrieve the MediaLink connecting two pads, identified by entity > > * names and pad indexes > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > > index fc31c52c0ecd..ed8228bb2fc6 100644 > > --- a/src/libcamera/pipeline/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo.cpp > > @@ -146,13 +146,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > > > > media_->acquire(); > > > > - for (MediaEntity *entity : media_->entities()) { > > - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { > > - video_ = new V4L2Device(entity); > > - break; > > - } > > - } > > + MediaEntity *entity = media_->defaultEntity(); > > + if (!entity) > > Looking at existing code if you don't find any DEFAULT entity, you > return false as you do here, but also release the media device. > > Should the same be done here? I think it should, and a message should likely be logged too. > > + return false; > > > > + video_ = new V4L2Device(entity); > > if (!video_ || video_->open()) { > > if (!video_) > > LOG(UVC, Error) << "Could not find a default video device"; > > diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp > > index 18d014caf4c8..dd28cccada6b 100644 > > --- a/test/v4l2_device/v4l2_device_test.cpp > > +++ b/test/v4l2_device/v4l2_device_test.cpp > > @@ -46,15 +46,13 @@ int V4L2DeviceTest::init() > > > > media_->acquire(); > > > > - for (MediaEntity *entity : media_->entities()) { > > - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { > > - dev_ = new V4L2Device(entity); > > - break; > > - } > > - } > > + MediaEntity *entity = media_->defaultEntity(); > > + if (!entity) > > + return TestFail; Note that we're planning to use vivid instead of uvcvideo for V4L2Device tests, so this may not be a good idea. > > + dev_ = new V4L2Device(entity); > > if (!dev_) > > - return TestSkip; > > + return TestFail; > > > > return dev_->open(); > > }
Hi Jacopo, On 07/02/2019 22:06, Jacopo Mondi wrote: > Hi Kieran, > ignorant question, is FL_DEFAULT an UVC specific thing? I think it's a media controller thing. I'm equally ignorant. This code caused my pain of not being able to import the buffers when I was working on it in the morning, and it made sense to refactor it out so that it did not hide my issue, as 'getting the default entity' for whatever purpose seems like a common thing to do at the MediaDevice level. > > A quick grep on kernel sources returns: > > $ git grep FL_DEFAULT drivers/media/ > drivers/media/platform/omap3isp/ispresizer.c: res->video_out.video.entity.flags |= MEDIA_ENT_FL_DEFAULT; > drivers/media/usb/uvc/uvc_entity.c: entity->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT; > Yes, I saw a similar (I guess exactingly so) grep when I did this :) > So I guess not, as omap3 seems also omap3 uses that as well. > > If that's not a UVC specificity, I welcome this change. > > On Thu, Feb 07, 2019 at 09:21:15PM +0000, Kieran Bingham wrote: >> Add a helper to identify the default entity from a media device. >> >> Utilise it in the two places which iterate the entities manually, to allocated >> a V4L2Device from the default entity. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/include/media_device.h | 1 + >> src/libcamera/media_device.cpp | 15 +++++++++++++++ >> src/libcamera/pipeline/uvcvideo.cpp | 10 ++++------ >> test/v4l2_device/v4l2_device_test.cpp | 12 +++++------- >> 4 files changed, 25 insertions(+), 13 deletions(-) >> >> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h >> index 9f038093b2b2..361e8f4a4b86 100644 >> --- a/src/libcamera/include/media_device.h >> +++ b/src/libcamera/include/media_device.h >> @@ -42,6 +42,7 @@ public: >> >> const std::vector<MediaEntity *> &entities() const { return entities_; } >> MediaEntity *getEntityByName(const std::string &name) const; >> + MediaEntity *defaultEntity() const; >> >> MediaLink *link(const std::string &sourceName, unsigned int sourceIdx, >> const std::string &sinkName, unsigned int sinkIdx); >> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp >> index 800ed330fe6d..4af90e1590a1 100644 >> --- a/src/libcamera/media_device.cpp >> +++ b/src/libcamera/media_device.cpp >> @@ -319,6 +319,21 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const >> return nullptr; >> } >> >> +/** >> + * \brief Return the default MediaEntity within a MediaDevice >> + * \return The default entity if specified, or a nullptr otherwise >> + */ >> +MediaEntity *MediaDevice::defaultEntity() const >> +{ >> + for (MediaEntity *entity : entities_) { >> + if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { >> + return entity; >> + } > > You could drop the inner braces { } here Ack. >> + } >> + >> + return nullptr; >> +} >> + >> /** >> * \brief Retrieve the MediaLink connecting two pads, identified by entity >> * names and pad indexes >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp >> index fc31c52c0ecd..ed8228bb2fc6 100644 >> --- a/src/libcamera/pipeline/uvcvideo.cpp >> +++ b/src/libcamera/pipeline/uvcvideo.cpp >> @@ -146,13 +146,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) >> >> media_->acquire(); >> >> - for (MediaEntity *entity : media_->entities()) { >> - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { >> - video_ = new V4L2Device(entity); >> - break; >> - } >> - } >> + MediaEntity *entity = media_->defaultEntity(); >> + if (!entity) > > Looking at existing code if you don't find any DEFAULT entity, you > return false as you do here, but also release the media device. > > Should the same be done here? > Ah yes, quite likely. >> + return false; >> >> + video_ = new V4L2Device(entity); >> if (!video_ || video_->open()) { >> if (!video_) >> LOG(UVC, Error) << "Could not find a default video device"; >> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp >> index 18d014caf4c8..dd28cccada6b 100644 >> --- a/test/v4l2_device/v4l2_device_test.cpp >> +++ b/test/v4l2_device/v4l2_device_test.cpp >> @@ -46,15 +46,13 @@ int V4L2DeviceTest::init() >> >> media_->acquire(); >> >> - for (MediaEntity *entity : media_->entities()) { >> - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { >> - dev_ = new V4L2Device(entity); >> - break; >> - } >> - } >> + MediaEntity *entity = media_->defaultEntity(); >> + if (!entity) >> + return TestFail; >> >> + dev_ = new V4L2Device(entity); >> if (!dev_) >> - return TestSkip; >> + return TestFail; >> >> return dev_->open(); >> } >> -- >> 2.19.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On 08/02/2019 15:44, Laurent Pinchart wrote: > Hello, > > On Thu, Feb 07, 2019 at 11:06:20PM +0100, Jacopo Mondi wrote: >> Hi Kieran, >> ignorant question, is FL_DEFAULT an UVC specific thing? > > It isn't, but it isn't widely used either. It would thus be fine using > the default entity in UVC-specific tests or in the UVC pipeline handler, > but not in the rest of the code, at least not without a fallback. The only places that use this helper are the uvcvideo pipeline, and the V4L2DeviceTest which is currently coded to skip if not operated on a UVC device... When this stops being coded to a UVC device - the helper can be removed or updated as necessary. >> A quick grep on kernel sources returns: >> >> $ git grep FL_DEFAULT drivers/media/ >> drivers/media/platform/omap3isp/ispresizer.c: res->video_out.video.entity.flags |= MEDIA_ENT_FL_DEFAULT; >> drivers/media/usb/uvc/uvc_entity.c: entity->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT; >> >> So I guess not, as omap3 seems also omap3 uses that as well. >> >> If that's not a UVC specificity, I welcome this change. >> >> On Thu, Feb 07, 2019 at 09:21:15PM +0000, Kieran Bingham wrote: >>> Add a helper to identify the default entity from a media device. >>> >>> Utilise it in the two places which iterate the entities manually, to allocated >>> a V4L2Device from the default entity. >>> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> src/libcamera/include/media_device.h | 1 + >>> src/libcamera/media_device.cpp | 15 +++++++++++++++ >>> src/libcamera/pipeline/uvcvideo.cpp | 10 ++++------ >>> test/v4l2_device/v4l2_device_test.cpp | 12 +++++------- >>> 4 files changed, 25 insertions(+), 13 deletions(-) >>> >>> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h >>> index 9f038093b2b2..361e8f4a4b86 100644 >>> --- a/src/libcamera/include/media_device.h >>> +++ b/src/libcamera/include/media_device.h >>> @@ -42,6 +42,7 @@ public: >>> >>> const std::vector<MediaEntity *> &entities() const { return entities_; } >>> MediaEntity *getEntityByName(const std::string &name) const; >>> + MediaEntity *defaultEntity() const; > > This should take an entity type as a parameter as the default flag is > defined as "Default entity for its type". It could also be used, for > instance, to discover the default camera sensor. I don't understand how that would be implemented. What extra item does it then check against? the entity type? >>> MediaLink *link(const std::string &sourceName, unsigned int sourceIdx, >>> const std::string &sinkName, unsigned int sinkIdx); >>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp >>> index 800ed330fe6d..4af90e1590a1 100644 >>> --- a/src/libcamera/media_device.cpp >>> +++ b/src/libcamera/media_device.cpp >>> @@ -319,6 +319,21 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const >>> return nullptr; >>> } >>> >>> +/** >>> + * \brief Return the default MediaEntity within a MediaDevice >>> + * \return The default entity if specified, or a nullptr otherwise >>> + */ >>> +MediaEntity *MediaDevice::defaultEntity() const >>> +{ >>> + for (MediaEntity *entity : entities_) { >>> + if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { >>> + return entity; >>> + } >> >> You could drop the inner braces { } here >>> + } >>> + >>> + return nullptr; >>> +} >>> + >>> /** >>> * \brief Retrieve the MediaLink connecting two pads, identified by entity >>> * names and pad indexes >>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp >>> index fc31c52c0ecd..ed8228bb2fc6 100644 >>> --- a/src/libcamera/pipeline/uvcvideo.cpp >>> +++ b/src/libcamera/pipeline/uvcvideo.cpp >>> @@ -146,13 +146,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) >>> >>> media_->acquire(); >>> >>> - for (MediaEntity *entity : media_->entities()) { >>> - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { >>> - video_ = new V4L2Device(entity); >>> - break; >>> - } >>> - } >>> + MediaEntity *entity = media_->defaultEntity(); >>> + if (!entity) >> >> Looking at existing code if you don't find any DEFAULT entity, you >> return false as you do here, but also release the media device. >> >> Should the same be done here? > > I think it should, and a message should likely be logged too. Ack. > >>> + return false; >>> >>> + video_ = new V4L2Device(entity); >>> if (!video_ || video_->open()) { >>> if (!video_) >>> LOG(UVC, Error) << "Could not find a default video device"; >>> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp >>> index 18d014caf4c8..dd28cccada6b 100644 >>> --- a/test/v4l2_device/v4l2_device_test.cpp >>> +++ b/test/v4l2_device/v4l2_device_test.cpp >>> @@ -46,15 +46,13 @@ int V4L2DeviceTest::init() >>> >>> media_->acquire(); >>> >>> - for (MediaEntity *entity : media_->entities()) { >>> - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { >>> - dev_ = new V4L2Device(entity); >>> - break; >>> - } >>> - } >>> + MediaEntity *entity = media_->defaultEntity(); >>> + if (!entity) >>> + return TestFail; > > Note that we're planning to use vivid instead of uvcvideo for V4L2Device > tests, so this may not be a good idea. Ok - so that should change at that point. Here to me it's a failure, because we will not know which UVC device to check against. I can add a comment to say that this check is UVC specific. >>> + dev_ = new V4L2Device(entity); >>> if (!dev_) >>> - return TestSkip; >>> + return TestFail; >>> >>> return dev_->open(); >>> } >
Hi Kieran, On Mon, Feb 11, 2019 at 11:47:35AM +0000, Kieran Bingham wrote: > On 08/02/2019 15:44, Laurent Pinchart wrote: > > On Thu, Feb 07, 2019 at 11:06:20PM +0100, Jacopo Mondi wrote: > >> Hi Kieran, > >> ignorant question, is FL_DEFAULT an UVC specific thing? > > > > It isn't, but it isn't widely used either. It would thus be fine using > > the default entity in UVC-specific tests or in the UVC pipeline handler, > > but not in the rest of the code, at least not without a fallback. > > The only places that use this helper are the uvcvideo pipeline, and the There it makes complete sense. > V4L2DeviceTest which is currently coded to skip if not operated on a UVC > device... > > When this stops being coded to a UVC device - the helper can be removed > or updated as necessary. Fine with me. > >> A quick grep on kernel sources returns: > >> > >> $ git grep FL_DEFAULT drivers/media/ > >> drivers/media/platform/omap3isp/ispresizer.c: res->video_out.video.entity.flags |= MEDIA_ENT_FL_DEFAULT; > >> drivers/media/usb/uvc/uvc_entity.c: entity->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT; > >> > >> So I guess not, as omap3 seems also omap3 uses that as well. > >> > >> If that's not a UVC specificity, I welcome this change. > >> > >> On Thu, Feb 07, 2019 at 09:21:15PM +0000, Kieran Bingham wrote: > >>> Add a helper to identify the default entity from a media device. > >>> > >>> Utilise it in the two places which iterate the entities manually, to allocated > >>> a V4L2Device from the default entity. > >>> > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> src/libcamera/include/media_device.h | 1 + > >>> src/libcamera/media_device.cpp | 15 +++++++++++++++ > >>> src/libcamera/pipeline/uvcvideo.cpp | 10 ++++------ > >>> test/v4l2_device/v4l2_device_test.cpp | 12 +++++------- > >>> 4 files changed, 25 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h > >>> index 9f038093b2b2..361e8f4a4b86 100644 > >>> --- a/src/libcamera/include/media_device.h > >>> +++ b/src/libcamera/include/media_device.h > >>> @@ -42,6 +42,7 @@ public: > >>> > >>> const std::vector<MediaEntity *> &entities() const { return entities_; } > >>> MediaEntity *getEntityByName(const std::string &name) const; > >>> + MediaEntity *defaultEntity() const; > > > > This should take an entity type as a parameter as the default flag is > > defined as "Default entity for its type". It could also be used, for > > instance, to discover the default camera sensor. > > I don't understand how that would be implemented. > > What extra item does it then check against? the entity type? The entity type, yes. > >>> MediaLink *link(const std::string &sourceName, unsigned int sourceIdx, > >>> const std::string &sinkName, unsigned int sinkIdx); > >>> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > >>> index 800ed330fe6d..4af90e1590a1 100644 > >>> --- a/src/libcamera/media_device.cpp > >>> +++ b/src/libcamera/media_device.cpp > >>> @@ -319,6 +319,21 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const > >>> return nullptr; > >>> } > >>> > >>> +/** > >>> + * \brief Return the default MediaEntity within a MediaDevice > >>> + * \return The default entity if specified, or a nullptr otherwise > >>> + */ > >>> +MediaEntity *MediaDevice::defaultEntity() const > >>> +{ > >>> + for (MediaEntity *entity : entities_) { > >>> + if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { > >>> + return entity; > >>> + } > >> > >> You could drop the inner braces { } here > >>> + } > >>> + > >>> + return nullptr; > >>> +} > >>> + > >>> /** > >>> * \brief Retrieve the MediaLink connecting two pads, identified by entity > >>> * names and pad indexes > >>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > >>> index fc31c52c0ecd..ed8228bb2fc6 100644 > >>> --- a/src/libcamera/pipeline/uvcvideo.cpp > >>> +++ b/src/libcamera/pipeline/uvcvideo.cpp > >>> @@ -146,13 +146,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > >>> > >>> media_->acquire(); > >>> > >>> - for (MediaEntity *entity : media_->entities()) { > >>> - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { > >>> - video_ = new V4L2Device(entity); > >>> - break; > >>> - } > >>> - } > >>> + MediaEntity *entity = media_->defaultEntity(); > >>> + if (!entity) > >> > >> Looking at existing code if you don't find any DEFAULT entity, you > >> return false as you do here, but also release the media device. > >> > >> Should the same be done here? > > > > I think it should, and a message should likely be logged too. > > Ack. > > >>> + return false; > >>> > >>> + video_ = new V4L2Device(entity); > >>> if (!video_ || video_->open()) { > >>> if (!video_) > >>> LOG(UVC, Error) << "Could not find a default video device"; > >>> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp > >>> index 18d014caf4c8..dd28cccada6b 100644 > >>> --- a/test/v4l2_device/v4l2_device_test.cpp > >>> +++ b/test/v4l2_device/v4l2_device_test.cpp > >>> @@ -46,15 +46,13 @@ int V4L2DeviceTest::init() > >>> > >>> media_->acquire(); > >>> > >>> - for (MediaEntity *entity : media_->entities()) { > >>> - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { > >>> - dev_ = new V4L2Device(entity); > >>> - break; > >>> - } > >>> - } > >>> + MediaEntity *entity = media_->defaultEntity(); > >>> + if (!entity) > >>> + return TestFail; > > > > Note that we're planning to use vivid instead of uvcvideo for V4L2Device > > tests, so this may not be a good idea. > > Ok - so that should change at that point. Here to me it's a failure, > because we will not know which UVC device to check against. > > I can add a comment to say that this check is UVC specific. That should be enough for now. > >>> + dev_ = new V4L2Device(entity); > >>> if (!dev_) > >>> - return TestSkip; > >>> + return TestFail; > >>> > >>> return dev_->open(); > >>> }
diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h index 9f038093b2b2..361e8f4a4b86 100644 --- a/src/libcamera/include/media_device.h +++ b/src/libcamera/include/media_device.h @@ -42,6 +42,7 @@ public: const std::vector<MediaEntity *> &entities() const { return entities_; } MediaEntity *getEntityByName(const std::string &name) const; + MediaEntity *defaultEntity() const; MediaLink *link(const std::string &sourceName, unsigned int sourceIdx, const std::string &sinkName, unsigned int sinkIdx); diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp index 800ed330fe6d..4af90e1590a1 100644 --- a/src/libcamera/media_device.cpp +++ b/src/libcamera/media_device.cpp @@ -319,6 +319,21 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const return nullptr; } +/** + * \brief Return the default MediaEntity within a MediaDevice + * \return The default entity if specified, or a nullptr otherwise + */ +MediaEntity *MediaDevice::defaultEntity() const +{ + for (MediaEntity *entity : entities_) { + if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { + return entity; + } + } + + return nullptr; +} + /** * \brief Retrieve the MediaLink connecting two pads, identified by entity * names and pad indexes diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index fc31c52c0ecd..ed8228bb2fc6 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -146,13 +146,11 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) media_->acquire(); - for (MediaEntity *entity : media_->entities()) { - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { - video_ = new V4L2Device(entity); - break; - } - } + MediaEntity *entity = media_->defaultEntity(); + if (!entity) + return false; + video_ = new V4L2Device(entity); if (!video_ || video_->open()) { if (!video_) LOG(UVC, Error) << "Could not find a default video device"; diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp index 18d014caf4c8..dd28cccada6b 100644 --- a/test/v4l2_device/v4l2_device_test.cpp +++ b/test/v4l2_device/v4l2_device_test.cpp @@ -46,15 +46,13 @@ int V4L2DeviceTest::init() media_->acquire(); - for (MediaEntity *entity : media_->entities()) { - if (entity->flags() & MEDIA_ENT_FL_DEFAULT) { - dev_ = new V4L2Device(entity); - break; - } - } + MediaEntity *entity = media_->defaultEntity(); + if (!entity) + return TestFail; + dev_ = new V4L2Device(entity); if (!dev_) - return TestSkip; + return TestFail; return dev_->open(); }
Add a helper to identify the default entity from a media device. Utilise it in the two places which iterate the entities manually, to allocated a V4L2Device from the default entity. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/include/media_device.h | 1 + src/libcamera/media_device.cpp | 15 +++++++++++++++ src/libcamera/pipeline/uvcvideo.cpp | 10 ++++------ test/v4l2_device/v4l2_device_test.cpp | 12 +++++------- 4 files changed, 25 insertions(+), 13 deletions(-)