[v2,01/35] libcamera: v4l2: Support fromEntityName with shared_ptr<MediaDevice>
diff mbox series

Message ID 20251023144841.403689-2-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Full dewarper support on imx8mp
Related show

Commit Message

Stefan Klug Oct. 23, 2025, 2:48 p.m. UTC 
From: Kieran Bingham <kieran.bingham@ideasonboard.com>

To facilitate expanding the use of a shared_ptr<MediaDevice> in
preference of a raw MediaDevice* pointer, add an additional
implementation for fromEntityName for both of v4l2_videodevice and
v4l2_subdevice to support this type.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 include/libcamera/internal/v4l2_subdevice.h   |  2 ++
 include/libcamera/internal/v4l2_videodevice.h |  2 ++
 src/libcamera/v4l2_subdevice.cpp              | 15 +++++++++++++++
 src/libcamera/v4l2_videodevice.cpp            | 15 +++++++++++++++
 4 files changed, 34 insertions(+)

Comments

Isaac Scott Oct. 24, 2025, 12:58 p.m. UTC | #1 
Hi Stefan,

Thank you for the patch!

Quoting Stefan Klug (2025-10-23 15:48:02)
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> To facilitate expanding the use of a shared_ptr<MediaDevice> in
> preference of a raw MediaDevice* pointer, add an additional
> implementation for fromEntityName for both of v4l2_videodevice and
> v4l2_subdevice to support this type.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_subdevice.h   |  2 ++
>  include/libcamera/internal/v4l2_videodevice.h |  2 ++
>  src/libcamera/v4l2_subdevice.cpp              | 15 +++++++++++++++
>  src/libcamera/v4l2_videodevice.cpp            | 15 +++++++++++++++
>  4 files changed, 34 insertions(+)
> 
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index c1cde1df2e36..d1c8b838d49b 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -163,6 +163,8 @@ public:
>  
>         static std::unique_ptr<V4L2Subdevice>
>         fromEntityName(const MediaDevice *media, const std::string &entity);
> +       static std::unique_ptr<V4L2Subdevice>
> +       fromEntityName(std::shared_ptr<const MediaDevice>, const std::string &entity);
>  
>  protected:
>         std::string logPrefix() const override;
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 6caafc4dcf08..4f80f7176174 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -228,6 +228,8 @@ public:
>  
>         static std::unique_ptr<V4L2VideoDevice>
>         fromEntityName(const MediaDevice *media, const std::string &entity);
> +       static std::unique_ptr<V4L2VideoDevice>
> +       fromEntityName(std::shared_ptr<const MediaDevice>, const std::string &entity);
>  
>         V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const;
>  
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 31a2ac72a7ee..1825e9e55ca4 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -1768,6 +1768,21 @@ V4L2Subdevice::fromEntityName(const MediaDevice *media,
>         return std::make_unique<V4L2Subdevice>(mediaEntity);
>  }
>  
> +/**
> + * \brief Create a new video subdevice instance from \a entity in media device
> + * \a media
> + * \param[in] media The media device where the entity is registered
> + * \param[in] entity The media entity name
> + *
> + * \return A newly created V4L2Subdevice on success, nullptr otherwise
> + */
> +std::unique_ptr<V4L2Subdevice>
> +V4L2Subdevice::fromEntityName(std::shared_ptr<const MediaDevice> media,
> +                             const std::string &entity)
> +{
> +       return fromEntityName(media.get(), entity);
> +}
> +
>  std::string V4L2Subdevice::logPrefix() const
>  {
>         return "'" + entity_->name() + "'";
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 7b48d911db73..9be5cfbc0395 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -2114,6 +2114,21 @@ V4L2VideoDevice::fromEntityName(const MediaDevice *media,
>         return std::make_unique<V4L2VideoDevice>(mediaEntity);
>  }
>  
> +/**
> + * \brief Create a new video device instance from \a entity in media device
> + * \a media
> + * \param[in] media The media device where the entity is registered
> + * \param[in] entity The media entity name
> + *
> + * \return A newly created V4L2VideoDevice on success, nullptr otherwise
> + */
> +std::unique_ptr<V4L2VideoDevice>
> +V4L2VideoDevice::fromEntityName(std::shared_ptr<const MediaDevice> media,
> +                               const std::string &entity)
> +{
> +       return fromEntityName(media.get(), entity);
> +}

Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>

> +
>  /**
>   * \brief Convert \a PixelFormat to a V4L2PixelFormat supported by the device
>   * \param[in] pixelFormat The PixelFormat to convert
> -- 
> 2.48.1
>
Barnabás Pőcze Oct. 24, 2025, 1:39 p.m. UTC | #2 
Hi

2025. 10. 23. 16:48 keltezéssel, Stefan Klug írta:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> To facilitate expanding the use of a shared_ptr<MediaDevice> in
> preference of a raw MediaDevice* pointer, add an additional
> implementation for fromEntityName for both of v4l2_videodevice and
> v4l2_subdevice to support this type.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>   include/libcamera/internal/v4l2_subdevice.h   |  2 ++
>   include/libcamera/internal/v4l2_videodevice.h |  2 ++
>   src/libcamera/v4l2_subdevice.cpp              | 15 +++++++++++++++
>   src/libcamera/v4l2_videodevice.cpp            | 15 +++++++++++++++
>   4 files changed, 34 insertions(+)
> 
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index c1cde1df2e36..d1c8b838d49b 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -163,6 +163,8 @@ public:
>   
>   	static std::unique_ptr<V4L2Subdevice>
>   	fromEntityName(const MediaDevice *media, const std::string &entity);
> +	static std::unique_ptr<V4L2Subdevice>
> +	fromEntityName(std::shared_ptr<const MediaDevice>, const std::string &entity);

This should be `const std::shared_ptr<const MediaDevice> &` at least to avoid making a copy.

Also:

   template<typename T, std::enable_if_t<!std::is_convertible_v<const T&, const MediaDevice *>> * = nullptr>
   static auto
   fromEntityName(const T &ptr, const std::string &entity)
     -> decltype(fromEntityName(static_cast<const MediaDevice *>(std::addressof(*ptr)), entity))
   {
     return fromEntityName(static_cast<const MediaDevice *>(std::addressof(*ptr)), entity);
   }

and now it works with anything that has an appropriate `operator*`.

But generally it does not seem that inconvenient to have to retrieve the raw pointer. And
`fromEntityName()` really does not need `std::shared_ptr`. So unless I'm missing something
I think maybe we should keep the raw pointer overload in any case, and either (a) add a generic
overload or (b) add a specific overload for shared_ptr or (c) just manually use `.get()`.


Regards,
Barnabás Pőcze


>   
>   protected:
>   	std::string logPrefix() const override;
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 6caafc4dcf08..4f80f7176174 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -228,6 +228,8 @@ public:
>   
>   	static std::unique_ptr<V4L2VideoDevice>
>   	fromEntityName(const MediaDevice *media, const std::string &entity);
> +	static std::unique_ptr<V4L2VideoDevice>
> +	fromEntityName(std::shared_ptr<const MediaDevice>, const std::string &entity);
>   
>   	V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const;
>   
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 31a2ac72a7ee..1825e9e55ca4 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -1768,6 +1768,21 @@ V4L2Subdevice::fromEntityName(const MediaDevice *media,
>   	return std::make_unique<V4L2Subdevice>(mediaEntity);
>   }
>   
> +/**
> + * \brief Create a new video subdevice instance from \a entity in media device
> + * \a media
> + * \param[in] media The media device where the entity is registered
> + * \param[in] entity The media entity name
> + *
> + * \return A newly created V4L2Subdevice on success, nullptr otherwise
> + */
> +std::unique_ptr<V4L2Subdevice>
> +V4L2Subdevice::fromEntityName(std::shared_ptr<const MediaDevice> media,
> +			      const std::string &entity)
> +{
> +	return fromEntityName(media.get(), entity);
> +}
> +
>   std::string V4L2Subdevice::logPrefix() const
>   {
>   	return "'" + entity_->name() + "'";
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 7b48d911db73..9be5cfbc0395 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -2114,6 +2114,21 @@ V4L2VideoDevice::fromEntityName(const MediaDevice *media,
>   	return std::make_unique<V4L2VideoDevice>(mediaEntity);
>   }
>   
> +/**
> + * \brief Create a new video device instance from \a entity in media device
> + * \a media
> + * \param[in] media The media device where the entity is registered
> + * \param[in] entity The media entity name
> + *
> + * \return A newly created V4L2VideoDevice on success, nullptr otherwise
> + */
> +std::unique_ptr<V4L2VideoDevice>
> +V4L2VideoDevice::fromEntityName(std::shared_ptr<const MediaDevice> media,
> +				const std::string &entity)
> +{
> +	return fromEntityName(media.get(), entity);
> +}
> +
>   /**
>    * \brief Convert \a PixelFormat to a V4L2PixelFormat supported by the device
>    * \param[in] pixelFormat The PixelFormat to convert
Laurent Pinchart Oct. 28, 2025, 10:16 a.m. UTC | #3 
On Fri, Oct 24, 2025 at 03:39:30PM +0200, Barnabás Pőcze wrote:
> Hi
> 
> 2025. 10. 23. 16:48 keltezéssel, Stefan Klug írta:
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > To facilitate expanding the use of a shared_ptr<MediaDevice> in
> > preference of a raw MediaDevice* pointer, add an additional
> > implementation for fromEntityName for both of v4l2_videodevice and
> > v4l2_subdevice to support this type.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >   include/libcamera/internal/v4l2_subdevice.h   |  2 ++
> >   include/libcamera/internal/v4l2_videodevice.h |  2 ++
> >   src/libcamera/v4l2_subdevice.cpp              | 15 +++++++++++++++
> >   src/libcamera/v4l2_videodevice.cpp            | 15 +++++++++++++++
> >   4 files changed, 34 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > index c1cde1df2e36..d1c8b838d49b 100644
> > --- a/include/libcamera/internal/v4l2_subdevice.h
> > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > @@ -163,6 +163,8 @@ public:
> >   
> >   	static std::unique_ptr<V4L2Subdevice>
> >   	fromEntityName(const MediaDevice *media, const std::string &entity);
> > +	static std::unique_ptr<V4L2Subdevice>
> > +	fromEntityName(std::shared_ptr<const MediaDevice>, const std::string &entity);
> 
> This should be `const std::shared_ptr<const MediaDevice> &` at least to avoid making a copy.
> 
> Also:
> 
>    template<typename T, std::enable_if_t<!std::is_convertible_v<const T&, const MediaDevice *>> * = nullptr>
>    static auto
>    fromEntityName(const T &ptr, const std::string &entity)
>      -> decltype(fromEntityName(static_cast<const MediaDevice *>(std::addressof(*ptr)), entity))
>    {
>      return fromEntityName(static_cast<const MediaDevice *>(std::addressof(*ptr)), entity);
>    }
> 
> and now it works with anything that has an appropriate `operator*`.
> 
> But generally it does not seem that inconvenient to have to retrieve the raw pointer. And
> `fromEntityName()` really does not need `std::shared_ptr`. So unless I'm missing something
> I think maybe we should keep the raw pointer overload in any case, and either (a) add a generic
> overload or (b) add a specific overload for shared_ptr or (c) just manually use `.get()`.

I agree, I don't think adding a .get() or * in the caller is a big
issue. As the fromEntityName() function does not need a shared pointer,
I wouldn't add this overload.

> >   
> >   protected:
> >   	std::string logPrefix() const override;
> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > index 6caafc4dcf08..4f80f7176174 100644
> > --- a/include/libcamera/internal/v4l2_videodevice.h
> > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > @@ -228,6 +228,8 @@ public:
> >   
> >   	static std::unique_ptr<V4L2VideoDevice>
> >   	fromEntityName(const MediaDevice *media, const std::string &entity);
> > +	static std::unique_ptr<V4L2VideoDevice>
> > +	fromEntityName(std::shared_ptr<const MediaDevice>, const std::string &entity);
> >   
> >   	V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const;
> >   
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index 31a2ac72a7ee..1825e9e55ca4 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -1768,6 +1768,21 @@ V4L2Subdevice::fromEntityName(const MediaDevice *media,
> >   	return std::make_unique<V4L2Subdevice>(mediaEntity);
> >   }
> >   
> > +/**
> > + * \brief Create a new video subdevice instance from \a entity in media device
> > + * \a media
> > + * \param[in] media The media device where the entity is registered
> > + * \param[in] entity The media entity name
> > + *
> > + * \return A newly created V4L2Subdevice on success, nullptr otherwise
> > + */
> > +std::unique_ptr<V4L2Subdevice>
> > +V4L2Subdevice::fromEntityName(std::shared_ptr<const MediaDevice> media,
> > +			      const std::string &entity)
> > +{
> > +	return fromEntityName(media.get(), entity);
> > +}
> > +
> >   std::string V4L2Subdevice::logPrefix() const
> >   {
> >   	return "'" + entity_->name() + "'";
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 7b48d911db73..9be5cfbc0395 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -2114,6 +2114,21 @@ V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> >   	return std::make_unique<V4L2VideoDevice>(mediaEntity);
> >   }
> >   
> > +/**
> > + * \brief Create a new video device instance from \a entity in media device
> > + * \a media
> > + * \param[in] media The media device where the entity is registered
> > + * \param[in] entity The media entity name
> > + *
> > + * \return A newly created V4L2VideoDevice on success, nullptr otherwise
> > + */
> > +std::unique_ptr<V4L2VideoDevice>
> > +V4L2VideoDevice::fromEntityName(std::shared_ptr<const MediaDevice> media,
> > +				const std::string &entity)
> > +{
> > +	return fromEntityName(media.get(), entity);
> > +}
> > +
> >   /**
> >    * \brief Convert \a PixelFormat to a V4L2PixelFormat supported by the device
> >    * \param[in] pixelFormat The PixelFormat to convert

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
index c1cde1df2e36..d1c8b838d49b 100644
--- a/include/libcamera/internal/v4l2_subdevice.h
+++ b/include/libcamera/internal/v4l2_subdevice.h
@@ -163,6 +163,8 @@  public:
 
 	static std::unique_ptr<V4L2Subdevice>
 	fromEntityName(const MediaDevice *media, const std::string &entity);
+	static std::unique_ptr<V4L2Subdevice>
+	fromEntityName(std::shared_ptr<const MediaDevice>, const std::string &entity);
 
 protected:
 	std::string logPrefix() const override;
diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index 6caafc4dcf08..4f80f7176174 100644
--- a/include/libcamera/internal/v4l2_videodevice.h
+++ b/include/libcamera/internal/v4l2_videodevice.h
@@ -228,6 +228,8 @@  public:
 
 	static std::unique_ptr<V4L2VideoDevice>
 	fromEntityName(const MediaDevice *media, const std::string &entity);
+	static std::unique_ptr<V4L2VideoDevice>
+	fromEntityName(std::shared_ptr<const MediaDevice>, const std::string &entity);
 
 	V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat) const;
 
diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
index 31a2ac72a7ee..1825e9e55ca4 100644
--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -1768,6 +1768,21 @@  V4L2Subdevice::fromEntityName(const MediaDevice *media,
 	return std::make_unique<V4L2Subdevice>(mediaEntity);
 }
 
+/**
+ * \brief Create a new video subdevice instance from \a entity in media device
+ * \a media
+ * \param[in] media The media device where the entity is registered
+ * \param[in] entity The media entity name
+ *
+ * \return A newly created V4L2Subdevice on success, nullptr otherwise
+ */
+std::unique_ptr<V4L2Subdevice>
+V4L2Subdevice::fromEntityName(std::shared_ptr<const MediaDevice> media,
+			      const std::string &entity)
+{
+	return fromEntityName(media.get(), entity);
+}
+
 std::string V4L2Subdevice::logPrefix() const
 {
 	return "'" + entity_->name() + "'";
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 7b48d911db73..9be5cfbc0395 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -2114,6 +2114,21 @@  V4L2VideoDevice::fromEntityName(const MediaDevice *media,
 	return std::make_unique<V4L2VideoDevice>(mediaEntity);
 }
 
+/**
+ * \brief Create a new video device instance from \a entity in media device
+ * \a media
+ * \param[in] media The media device where the entity is registered
+ * \param[in] entity The media entity name
+ *
+ * \return A newly created V4L2VideoDevice on success, nullptr otherwise
+ */
+std::unique_ptr<V4L2VideoDevice>
+V4L2VideoDevice::fromEntityName(std::shared_ptr<const MediaDevice> media,
+				const std::string &entity)
+{
+	return fromEntityName(media.get(), entity);
+}
+
 /**
  * \brief Convert \a PixelFormat to a V4L2PixelFormat supported by the device
  * \param[in] pixelFormat The PixelFormat to convert