[libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject class hierarchy

[Jacopo Mondi]

Hi Niklas,

On Sun, Dec 30, 2018 at 11:47:56PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2018-12-30 23:10:57 +0100, Jacopo Mondi wrote:
> > Hi Niklas,
> >    thanks for review
> >
> > On Sun, Dec 30, 2018 at 10:50:15PM +0100, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your patch.
> > >
> > > On 2018-12-30 15:23:12 +0100, Jacopo Mondi wrote:
> > > > Add a class hierarcy to represent all media objects a media graph represents.
> > > > Add a base MediaObject class, which retains the global unique object id,
> > > > and define the derived MediaEntity, MediaLink and MediaPad classes.
> > > >
> > > > This hierarchy will be used by the MediaDevice objects which represents and
> > > > handles the media graph.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  src/libcamera/include/media_object.h | 107 ++++++++++
> > > >  src/libcamera/media_object.cpp       | 281 +++++++++++++++++++++++++++
> > > >  src/libcamera/meson.build            |   2 +
> > > >  3 files changed, 390 insertions(+)
> > > >  create mode 100644 src/libcamera/include/media_object.h
> > > >  create mode 100644 src/libcamera/media_object.cpp
> > > >
> > > > diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h
> > > > new file mode 100644
> > > > index 0000000..118d0d8
> > > > --- /dev/null
> > > > +++ b/src/libcamera/include/media_object.h
> > > > @@ -0,0 +1,107 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2018, Google Inc.
> > > > + *
> > > > + * media_object.h - Media Device objects: entities, pads and links.
> > > > + */
> > > > +#ifndef __LIBCAMERA_MEDIA_OBJECT_H__
> > > > +#define __LIBCAMERA_MEDIA_OBJECT_H__
> > > > +
> > > > +#include <string>
> > > > +#include <vector>
> > > > +
> > > > +#include <linux/media.h>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +class MediaDevice;
> > > > +class MediaEntity;
> > > > +
> > > > +class MediaObject
> > > > +{
> > > > +public:
> > > > +	MediaObject(unsigned int id) : id_(id) { }
> > > > +	virtual ~MediaObject() { }
> > > > +
> > > > +	unsigned int id() const { return id_; }
> > > > +
> > > > +protected:
> > > > +	unsigned int id_;
> > > > +};
> > > > +
> > > > +class MediaPad;
> > > > +class MediaLink : public MediaObject
> > > > +{
> > > > +	friend class MediaDevice;
> > > > +
> > > > +public:
> > > > +	~MediaLink() { }
> > > > +
> > > > +	MediaPad *source() const { return source_; }
> > > > +	MediaPad *sink() const { return sink_; }
> > > > +	unsigned int flags() const { return flags_; }
> > > > +	void setFlags(unsigned int flags) { flags_ = flags; }
> > > > +
> > > > +private:
> > > > +	MediaLink(const struct media_v2_link *link,
> > > > +		  MediaPad *source, MediaPad *sink);
> > >
> > > Why not use references here? Would it be valid to create a MediaLink
> > > where any of the parameters are nullptr?
> >
> > I feel it is better to use pointers for dynamically allocated objects
> > as this class entities are. The caller will have pointers to them, and
> > I should dereference them to access their value and pass it by
> > reference.
> >
> > As I've replied to your series, I have not found this stated anywhere,
> > so it might just be my preference, but I agree this protects us from
> > nullptr values.
> >
> > >
> > > Same question for other constructors in this patch.
> > >
> >
> > Could I keep them like this until we don't try establish a firm rule?
>
> I have no problem keeping them, but if you want to keep them you should
> ensure they are not null before dereferencing them.
>

The caller checks for those pointers validity, and makes sure all
these objects are initialized with valid pointers.

Let me point out that using references, and thus accessing the
pointer's value, if the caller provides a nullptr won't help:

                MediaPad *source == nullprt;
                MediaPad *source == nullprt;
                new MediaLink(link, *source, *sink)

Will obviusly segault when the function is called.

Thanks
  j
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181231/932b564f/attachment.sig>