[libcamera-devel] [PATCH v2 1/4] libcamera: Add pointer to MediaDevice to MediaObject

[Laurent Pinchart]

Hi Jacopo,

On Tuesday, 8 January 2019 21:33:51 EET Jacopo Mondi wrote:
> On Tue, Jan 08, 2019 at 08:07:59PM +0200, Laurent Pinchart wrote:
> > On Tuesday, 8 January 2019 19:04:04 EET Jacopo Mondi wrote:
> >> Add a MediaDevice member field to the MediaObject class hierarcy.
> >> Each media object now has a reference to the media device it belongs to,
> >> and which it has been created by.
> >> 
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >> v1->v2:
> >> - Use the entity MediaDevice in MediaPad and MediaLink constructors
> >> - Incorporate comments changes from Laurent and Niklas on v1
> >> 
> >>  src/libcamera/include/media_object.h |  7 +++++--
> >>  src/libcamera/media_device.cpp       |  4 ++--
> >>  src/libcamera/media_object.cpp       | 27 +++++++++++++++++++--------
> >>  3 files changed, 26 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/src/libcamera/include/media_object.h
> >> b/src/libcamera/include/media_object.h index 04b9a89..d92aab3 100644
> >> --- a/src/libcamera/include/media_object.h
> >> +++ b/src/libcamera/include/media_object.h
> >> @@ -22,13 +22,16 @@ class MediaObject
> >> 
> >>  {
> >>  
> >>  public:
> >>  	unsigned int id() const { return id_; }
> >> 
> >> +	MediaDevice *dev() const { return dev_; }
> > 
> > For the reason explained in v1, I would make this either
> > 
> > 	MediaDevice *dev() { return dev_; }
> > 	const MediaDevice *dev() const { return dev_; }
> > 
> > or just
> > 
> > 	MediaDevice *dev() { return dev_; }
> 
> Ok, I might had missed that... a const function could be called on const
> instances, and if you get a link, then you probably want to modify it,
> and if you try to do so on a const MediaDevice instance, that will
> fail... Is this what you're trying to protect?

Using the proposed API, you can do MediaDevice::link(...) ->setEnabled() and 
that would modify the state of the MediaDevice, even if the MediaDevice 
instance was originally const. I don't think that's a good idea.

> > And I think you can spell the name of the function fully.
> 
> MediaDevice *device() {...}
> ?

Yes.

> >>  protected:
> >>  	friend class MediaDevice;
> >> 
> >> -	MediaObject(unsigned int id) : id_(id) { }
> >> +	MediaObject(MediaDevice *dev, unsigned int id) :
> >> +		dev_(dev), id_(id) { }
> >>  	virtual ~MediaObject() { }
> >> 
> >> +	MediaDevice *dev_;
> >>  	unsigned int id_;
> >>  };
> >> 
> >> @@ -93,7 +96,7 @@ public:
> >>  private:
> >>  	friend class MediaDevice;
> >> 
> >> -	MediaEntity(const struct media_v2_entity *entity,
> >> +	MediaEntity(MediaDevice *media, const struct media_v2_entity *entity,
> > 
> > If MediaObject is constructed with a MediaDevice *dev, should this be
> > MediaDevice *dev too ?
> 
> Yes, I missed that
> 
> >>  		    unsigned int major = 0, unsigned int minor = 0);
> >>  	MediaEntity(const MediaEntity &) = delete;
> >>  	~MediaEntity();
> >> 

[snip]

-- 
Regards,

Laurent Pinchart