[libcamera-devel,1/5] libcamera: Add support for ancillary links to MediaLink
diff mbox series

Message ID 20211126003118.42356-2-djrscally@gmail.com
State Superseded
Headers show
Series
  • Enumerate CameraLens by following sensor's ancillary links
Related show

Commit Message

Daniel Scally Nov. 26, 2021, 12:31 a.m. UTC 
Update the MediaLink class to include members suitable for the new
type of media_v2_link, connecting two instances of MediaEntity
rather than MediaPads

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---

Adding new members and a new constructor here seemed in the end like the least
impactful and probably cleanest method of doing this, as otherwise the source
and sink would need to become MediaObjects and be cast everywhere.

 include/libcamera/internal/media_object.h | 10 ++++++++++
 src/libcamera/media_object.cpp            | 24 ++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Nov. 28, 2021, 10:02 p.m. UTC | #1 
Hi Daniel,

Thank you for the patch.

On Fri, Nov 26, 2021 at 12:31:14AM +0000, Daniel Scally wrote:
> Update the MediaLink class to include members suitable for the new
> type of media_v2_link, connecting two instances of MediaEntity
> rather than MediaPads
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> 
> Adding new members and a new constructor here seemed in the end like the least
> impactful and probably cleanest method of doing this, as otherwise the source
> and sink would need to become MediaObjects and be cast everywhere.

I'm not really thrilled by this. Given that the purpose of ancillary
links is to expose the relationship between a primary device and its
ancillary devices, and given that those relationships don't carry any
other property than their existence, I think adding a

	const std::vector<MediaEntity *> ancillaryEntities() const;

(naming bikeshedding accepted) function to the MediaEntity class should
be enough, without a need to extend the MediaLink API.

>  include/libcamera/internal/media_object.h | 10 ++++++++++
>  src/libcamera/media_object.cpp            | 24 ++++++++++++++++++++++-
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h
> index 6ae22c67..79c71325 100644
> --- a/include/libcamera/internal/media_object.h
> +++ b/include/libcamera/internal/media_object.h
> @@ -45,6 +45,8 @@ class MediaLink : public MediaObject
>  public:
>  	MediaPad *source() const { return source_; }
>  	MediaPad *sink() const { return sink_; }
> +	MediaEntity *primary() const { return primary_; };
> +	MediaEntity *ancillary() const { return ancillary_; };
>  	unsigned int flags() const { return flags_; }
>  	int setEnabled(bool enable);
>  
> @@ -55,9 +57,13 @@ private:
>  
>  	MediaLink(const struct media_v2_link *link,
>  		  MediaPad *source, MediaPad *sink);
> +	MediaLink(const struct media_v2_link *link,
> +		  MediaEntity *primary, MediaEntity *ancillary);
>  
>  	MediaPad *source_;
>  	MediaPad *sink_;
> +	MediaEntity *primary_;
> +	MediaEntity *ancillary_;
>  	unsigned int flags_;
>  };
>  
> @@ -104,12 +110,15 @@ public:
>  	unsigned int deviceMinor() const { return minor_; }
>  
>  	const std::vector<MediaPad *> &pads() const { return pads_; }
> +	const std::vector<MediaLink *> &ancillary_links() const { return ancillary_links_; }
>  
>  	const MediaPad *getPadByIndex(unsigned int index) const;
>  	const MediaPad *getPadById(unsigned int id) const;
>  
>  	int setDeviceNode(const std::string &deviceNode);
>  
> +	void addLink(MediaLink *link);
> +
>  private:
>  	LIBCAMERA_DISABLE_COPY_AND_MOVE(MediaEntity)
>  
> @@ -129,6 +138,7 @@ private:
>  	unsigned int minor_;
>  
>  	std::vector<MediaPad *> pads_;
> +	std::vector<MediaLink *> ancillary_links_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> index f425d044..e903d5ef 100644
> --- a/src/libcamera/media_object.cpp
> +++ b/src/libcamera/media_object.cpp
> @@ -134,7 +134,7 @@ int MediaLink::setEnabled(bool enable)
>  }
>  
>  /**
> - * \brief Construct a MediaLink
> + * \brief Construct a MediaLink between two pads
>   * \param[in] link The media link kernel data
>   * \param[in] source The source pad at the origin of the link
>   * \param[in] sink The sink pad at the destination of the link
> @@ -146,6 +146,19 @@ MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,
>  {
>  }
>  
> +/**
> + * \brief Construct a MediaLink between two entities
> + * \param[in] link The media link kernel data
> + * \param[in] primary The primary entity at the origin of the link
> + * \param[in] ancillary The ancillary entity at the destination of the link
> + */
> +MediaLink::MediaLink(const struct media_v2_link *link, MediaEntity *primary,
> +		     MediaEntity *ancillary)
> +	: MediaObject(primary->device(), link->id), primary_(primary),
> +	  ancillary_(ancillary), flags_(link->flags)
> +{
> +}
> +
>  /**
>   * \fn MediaLink::source()
>   * \brief Retrieve the link's source pad
> @@ -378,6 +391,15 @@ int MediaEntity::setDeviceNode(const std::string &deviceNode)
>  	return 0;
>  }
>  
> +/**
> + * \brief Add an ancillary link to the MediaEntity
> + * \param[in] link Pointer to the MediaLink class
> + */
> +void MediaEntity::addLink(MediaLink *link)
> +{
> +	ancillary_links_.push_back(link);
> +}
> +
>  /**
>   * \brief Construct a MediaEntity
>   * \param[in] dev The media device this entity belongs to
Daniel Scally Nov. 29, 2021, 8:38 a.m. UTC | #2 
Morning Laurent

On 28/11/2021 22:02, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Fri, Nov 26, 2021 at 12:31:14AM +0000, Daniel Scally wrote:
>> Update the MediaLink class to include members suitable for the new
>> type of media_v2_link, connecting two instances of MediaEntity
>> rather than MediaPads
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>
>> Adding new members and a new constructor here seemed in the end like the least
>> impactful and probably cleanest method of doing this, as otherwise the source
>> and sink would need to become MediaObjects and be cast everywhere.
> I'm not really thrilled by this. Given that the purpose of ancillary
> links is to expose the relationship between a primary device and its
> ancillary devices, and given that those relationships don't carry any
> other property than their existence, I think adding a
>
> 	const std::vector<MediaEntity *> ancillaryEntities() const;
>
> (naming bikeshedding accepted) function to the MediaEntity class should
> be enough, without a need to extend the MediaLink API.


Fair enough...that would simplify patch two quite a lot too I suppose.
I'll switch to doing it that way for the v2.

>
>>  include/libcamera/internal/media_object.h | 10 ++++++++++
>>  src/libcamera/media_object.cpp            | 24 ++++++++++++++++++++++-
>>  2 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h
>> index 6ae22c67..79c71325 100644
>> --- a/include/libcamera/internal/media_object.h
>> +++ b/include/libcamera/internal/media_object.h
>> @@ -45,6 +45,8 @@ class MediaLink : public MediaObject
>>  public:
>>  	MediaPad *source() const { return source_; }
>>  	MediaPad *sink() const { return sink_; }
>> +	MediaEntity *primary() const { return primary_; };
>> +	MediaEntity *ancillary() const { return ancillary_; };
>>  	unsigned int flags() const { return flags_; }
>>  	int setEnabled(bool enable);
>>  
>> @@ -55,9 +57,13 @@ private:
>>  
>>  	MediaLink(const struct media_v2_link *link,
>>  		  MediaPad *source, MediaPad *sink);
>> +	MediaLink(const struct media_v2_link *link,
>> +		  MediaEntity *primary, MediaEntity *ancillary);
>>  
>>  	MediaPad *source_;
>>  	MediaPad *sink_;
>> +	MediaEntity *primary_;
>> +	MediaEntity *ancillary_;
>>  	unsigned int flags_;
>>  };
>>  
>> @@ -104,12 +110,15 @@ public:
>>  	unsigned int deviceMinor() const { return minor_; }
>>  
>>  	const std::vector<MediaPad *> &pads() const { return pads_; }
>> +	const std::vector<MediaLink *> &ancillary_links() const { return ancillary_links_; }
>>  
>>  	const MediaPad *getPadByIndex(unsigned int index) const;
>>  	const MediaPad *getPadById(unsigned int id) const;
>>  
>>  	int setDeviceNode(const std::string &deviceNode);
>>  
>> +	void addLink(MediaLink *link);
>> +
>>  private:
>>  	LIBCAMERA_DISABLE_COPY_AND_MOVE(MediaEntity)
>>  
>> @@ -129,6 +138,7 @@ private:
>>  	unsigned int minor_;
>>  
>>  	std::vector<MediaPad *> pads_;
>> +	std::vector<MediaLink *> ancillary_links_;
>>  };
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
>> index f425d044..e903d5ef 100644
>> --- a/src/libcamera/media_object.cpp
>> +++ b/src/libcamera/media_object.cpp
>> @@ -134,7 +134,7 @@ int MediaLink::setEnabled(bool enable)
>>  }
>>  
>>  /**
>> - * \brief Construct a MediaLink
>> + * \brief Construct a MediaLink between two pads
>>   * \param[in] link The media link kernel data
>>   * \param[in] source The source pad at the origin of the link
>>   * \param[in] sink The sink pad at the destination of the link
>> @@ -146,6 +146,19 @@ MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,
>>  {
>>  }
>>  
>> +/**
>> + * \brief Construct a MediaLink between two entities
>> + * \param[in] link The media link kernel data
>> + * \param[in] primary The primary entity at the origin of the link
>> + * \param[in] ancillary The ancillary entity at the destination of the link
>> + */
>> +MediaLink::MediaLink(const struct media_v2_link *link, MediaEntity *primary,
>> +		     MediaEntity *ancillary)
>> +	: MediaObject(primary->device(), link->id), primary_(primary),
>> +	  ancillary_(ancillary), flags_(link->flags)
>> +{
>> +}
>> +
>>  /**
>>   * \fn MediaLink::source()
>>   * \brief Retrieve the link's source pad
>> @@ -378,6 +391,15 @@ int MediaEntity::setDeviceNode(const std::string &deviceNode)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * \brief Add an ancillary link to the MediaEntity
>> + * \param[in] link Pointer to the MediaLink class
>> + */
>> +void MediaEntity::addLink(MediaLink *link)
>> +{
>> +	ancillary_links_.push_back(link);
>> +}
>> +
>>  /**
>>   * \brief Construct a MediaEntity
>>   * \param[in] dev The media device this entity belongs to

Patch
diff mbox series 

diff --git a/include/libcamera/internal/media_object.h b/include/libcamera/internal/media_object.h
index 6ae22c67..79c71325 100644
--- a/include/libcamera/internal/media_object.h
+++ b/include/libcamera/internal/media_object.h
@@ -45,6 +45,8 @@  class MediaLink : public MediaObject
 public:
 	MediaPad *source() const { return source_; }
 	MediaPad *sink() const { return sink_; }
+	MediaEntity *primary() const { return primary_; };
+	MediaEntity *ancillary() const { return ancillary_; };
 	unsigned int flags() const { return flags_; }
 	int setEnabled(bool enable);
 
@@ -55,9 +57,13 @@  private:
 
 	MediaLink(const struct media_v2_link *link,
 		  MediaPad *source, MediaPad *sink);
+	MediaLink(const struct media_v2_link *link,
+		  MediaEntity *primary, MediaEntity *ancillary);
 
 	MediaPad *source_;
 	MediaPad *sink_;
+	MediaEntity *primary_;
+	MediaEntity *ancillary_;
 	unsigned int flags_;
 };
 
@@ -104,12 +110,15 @@  public:
 	unsigned int deviceMinor() const { return minor_; }
 
 	const std::vector<MediaPad *> &pads() const { return pads_; }
+	const std::vector<MediaLink *> &ancillary_links() const { return ancillary_links_; }
 
 	const MediaPad *getPadByIndex(unsigned int index) const;
 	const MediaPad *getPadById(unsigned int id) const;
 
 	int setDeviceNode(const std::string &deviceNode);
 
+	void addLink(MediaLink *link);
+
 private:
 	LIBCAMERA_DISABLE_COPY_AND_MOVE(MediaEntity)
 
@@ -129,6 +138,7 @@  private:
 	unsigned int minor_;
 
 	std::vector<MediaPad *> pads_;
+	std::vector<MediaLink *> ancillary_links_;
 };
 
 } /* namespace libcamera */
diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
index f425d044..e903d5ef 100644
--- a/src/libcamera/media_object.cpp
+++ b/src/libcamera/media_object.cpp
@@ -134,7 +134,7 @@  int MediaLink::setEnabled(bool enable)
 }
 
 /**
- * \brief Construct a MediaLink
+ * \brief Construct a MediaLink between two pads
  * \param[in] link The media link kernel data
  * \param[in] source The source pad at the origin of the link
  * \param[in] sink The sink pad at the destination of the link
@@ -146,6 +146,19 @@  MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,
 {
 }
 
+/**
+ * \brief Construct a MediaLink between two entities
+ * \param[in] link The media link kernel data
+ * \param[in] primary The primary entity at the origin of the link
+ * \param[in] ancillary The ancillary entity at the destination of the link
+ */
+MediaLink::MediaLink(const struct media_v2_link *link, MediaEntity *primary,
+		     MediaEntity *ancillary)
+	: MediaObject(primary->device(), link->id), primary_(primary),
+	  ancillary_(ancillary), flags_(link->flags)
+{
+}
+
 /**
  * \fn MediaLink::source()
  * \brief Retrieve the link's source pad
@@ -378,6 +391,15 @@  int MediaEntity::setDeviceNode(const std::string &deviceNode)
 	return 0;
 }
 
+/**
+ * \brief Add an ancillary link to the MediaEntity
+ * \param[in] link Pointer to the MediaLink class
+ */
+void MediaEntity::addLink(MediaLink *link)
+{
+	ancillary_links_.push_back(link);
+}
+
 /**
  * \brief Construct a MediaEntity
  * \param[in] dev The media device this entity belongs to